gmane.comp.video.libav.devel

[RFC] vorbis: drop a (dis)optimization and relativeassert

Luca Barbato — 2013-05-18T13:40:54

On recent hardware and compiler using a plain division is faster.
---

Some numbers using gcc-4.8 in -Og mode.

avconv
1399640 decicycles in vorbis, 2048 runs, 0 skips
avconv-div
1385903 decicycles in vorbis, 2048 runs, 0 skips
avconv
1422062 decicycles in vorbis, 2048 runs, 0 skips
avconv-div
1393888 decicycles in vorbis, 2048 runs, 0 skips
avconv
1420744 decicycles in vorbis, 2048 runs, 0 skips
avconv-div
1389816 decicycles in vorbis, 2048 runs, 0 skips
avconv
1398754 decicycles in vorbis, 2048 runs, 0 skips
avconv-div
1384642 decicycles in vorbis, 2048 runs, 0 skips
avconv
1408228 decicycles in vorbis, 2048 runs, 0 skips
avconv-div
1402553 decicycles in vorbis, 2048 runs, 0 skips
avconv
1404719 decicycles in vorbis, 2048 runs, 0 skips
avconv-div
1400232 decicycles in vorbis, 2048 runs, 0 skips


 libavcodec/vorbisdec.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
index 03c80b4..35e2242 100644
--- a/libavcodec/vorbisdec.c
+++ b/l

Re: [PATCH 1/3] lavfi: add a slice threadinginfrastructure

Luca Barbato — 2013-05-18T10:45:09

Looked fine after the first read, now I'm halfway the second.

Re: [PATCH 2/2] prores: update FATE test to account for alpha plane present in the test sample

Luca Barbato — 2013-05-18T09:55:30

Looks fine.

Re: [PATCH] build: do not leave asserts enabled

Luca Barbato — 2013-05-18T09:53:53

Usually we do, by fixing the bug.

Ideally some assert are completely impossible short of memory
corruption, some are possible and thus should be replaced with normal
error paths.


I'm against replacing plain asserts with av_asserts, ether they have no
sense, thus should be removed like commented out code, or they have
sense, thus should be replaced with normal error paths.

The only reason you should put asserts is when you refactor code so it
gets glaring obvious when you are doing wrong.

At the cost of being boring asserts have no place in released code, as
you said an assert is a bug and should be threaded accordingly.


And usually we fix the bugs =)

Moving to avassert is just like leaving known issues because of lack of
time.

lu

Re: [PATCH 2/3] vf_yadif: support slice threading

Hendrik Leppkes — 2013-05-18T08:18:58

The changes are trivial enough, LGTM.

Re: [PATCH] w32pthreads: move from lavc to compat/

Hendrik Leppkes — 2013-05-18T08:11:31


LGTM
Tested on mingw and msvc as well, compiles and runs fine.

[PATCH 2/2] prores: update FATE test to account for alpha plane present in the test sample

Kostya Shishkov — 2013-05-18T05:16:30

---
 tests/fate/prores.mak       |    2 +-
 tests/ref/fate/prores-alpha |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/fate/prores.mak b/tests/fate/prores.mak
index 9c1a1b1..ccc71af 100644
--- a/tests/fate/prores.mak
+++ b/tests/fate/prores.mak
< at >< at > -11,4 +11,4 < at >< at > fate-prores-422:       CMD = framecrc -i $(SAMPLES)/prores/Sequence_1-Apple_ProR
 fate-prores-422_hq:    CMD = framecrc -i $(SAMPLES)/prores/Sequence_1-Apple_ProRes_422_HQ.mov -pix_fmt yuv422p10le
 fate-prores-422_lt:    CMD = framecrc -i $(SAMPLES)/prores/Sequence_1-Apple_ProRes_422_LT.mov -pix_fmt yuv422p10le
 fate-prores-422_proxy: CMD = framecrc -i $(SAMPLES)/prores/Sequence_1-Apple_ProRes_422_Proxy.mov -pix_fmt yuv422p10le
-fate-prores-alpha:     CMD = framecrc -i $(SAMPLES)/prores/Sequence_1-Apple_ProRes_with_Alpha.mov -pix_fmt yuv444p10le
+fate-prores-alpha:     CMD = framecrc -i $(SAMPLES)/prores/Sequence_1-Apple_ProRes_with_Alpha.mov -pix_fmt yuva444p10le
diff --git a/tests/ref/fate/prores-alpha b/tests/ref

[PATCH 1/2] proresdec: simplify slice componentoffsets handling

Kostya Shishkov — 2013-05-18T05:16:04

---
I promised that after all...
---
 libavcodec/proresdec.c |   50 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/libavcodec/proresdec.c b/libavcodec/proresdec.c
index 0d1eb7a..8da2db6 100644
--- a/libavcodec/proresdec.c
+++ b/libavcodec/proresdec.c
< at >< at > -559,6 +559,7 < at >< at > static int decode_slice(AVCodecContext *avctx, void *tdata)
     int slice_data_size, hdr_size;
     int y_data_size, u_data_size, v_data_size, a_data_size;
     int y_linesize, u_linesize, v_linesize, a_linesize;
+    int coff[4];
 
     buf             = ctx->slice_data[slice_num].index;
     slice_data_size = ctx->slice_data[slice_num + 1].index - buf;
< at >< at > -586,6 +587,10 < at >< at > static int decode_slice(AVCodecContext *avctx, void *tdata)
         v_linesize <<= 1;
         a_linesize <<= 1;
     }
+    y_data += (mb_y_pos << 4) * y_linesize + (mb_x_pos << 5);
+    u_data += (mb_y_pos << 4) * u_linesize + (mb_x_pos << ctx->mb_chroma_factor);
+    v_data += (mb_y_pos << 4) * v_line

Re: [PATCH v1.1] Eidos Escape 130 decoder

Kostya Shishkov — 2013-05-18T05:14:51

Pushed the decoder, test will be pushed later (since it's not 24 hours since
you've added the sample).

Re: [PATCH] build: do not leave asserts enabled

Anton Khirnov — 2013-05-18T04:13:23

On Fri, 17 May 2013 22:04:22 +0200, Luca Barbato <lu_zero-aBrp7R+bbdUdnm+yROfE0A< at >public.gmane.org> wrote:

That is a bug in the decoder then, which should be fixed. It's not really
different from any other bug that would cause a crash.
Many kinds of code can cause a crash if there's a bug there, shall we remove all
such code?

That said, I'm not against replacing plain asserts with av_asserts, so people
who prefer random hard to debug crashes over asserts can choose that.
But I am against removing asserts on the grounds that 'it might crash if there
is a bug in it'. A lot of code might crash if there is a bug in it.

Re: [WIP 3/3] wavpack: do not read over the buffer boundary

Kostya Shishkov — 2013-05-18T03:59:53

I don't like it either and can suggest bytestream2

Re: [PATCH 2/3] wavpack: validate samples size parsed in wavpack_decode_block

Kostya Shishkov — 2013-05-18T03:56:27

they all should be the same, so !=


otherwise OK

Re: [PATCH 1/3] wavpack: return meaningful errors

Kostya Shishkov — 2013-05-18T03:55:25

LGTM

[WIP 3/3] wavpack: do not read over the bufferboundary

Luca Barbato — 2013-05-17T21:15:37

---

I don't like it incredibly but works. Anybody has a better idea?

 libavcodec/wavpack.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
index 233c533..06612ab 100644
--- a/libavcodec/wavpack.c
+++ b/libavcodec/wavpack.c
< at >< at > -902,19 +902,27 < at >< at > static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
             }
             t = 0;
             for (i = s->terms - 1; (i >= 0) && (t < size); i--) {
+                int dec_size =
+                    4 * (s->decorr[i].value > 8) * (1 + s->stereo_in) +
+                    4 * (s->decorr[i].value < 0) +
+                    2 *  s->decorr[i].value * (s->stereo_in + 1) *
+                    (s->decorr[i].value <= 8 && s->decorr[i].value >= 0);
+
+                if (buf + dec_size > buf_end)
+                    break;
+
+                t += dec_size;
+
                 if (s->decorr[i].value > 8) {
                     s->decorr[i].samplesA[0] = wp_exp2(AV_RL16(

[PATCH 1/3] wavpack: return meaningful errors

Luca Barbato — 2013-05-17T21:15:35

And forward those that were already meaningful.
---
 libavcodec/wavpack.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
index c74901d..999e532 100644
--- a/libavcodec/wavpack.c
+++ b/libavcodec/wavpack.c
< at >< at > -777,14 +777,14 < at >< at > static int wavpack_decode_block(AVCodecContext *avctx, int block_no,

     if (block_no >= wc->fdec_num && wv_alloc_frame_context(wc) < 0) {
         av_log(avctx, AV_LOG_ERROR, "Error creating frame decode context\n");
-        return -1;
+        return AVERROR_INVALIDDATA;
     }

     s = wc->fdec[block_no];
     if (!s) {
         av_log(avctx, AV_LOG_ERROR, "Context for block %d is not present\n",
                block_no);
-        return -1;
+        return AVERROR_INVALIDDATA;
     }

     memset(s->decorr, 0, MAX_TERMS * sizeof(Decorr));
< at >< at > -1034,7 +1034,7 < at >< at > static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
             if (size <= 1) {
                 av_l

[PATCH 2/3] wavpack: validate samples size parsed inwavpack_decode_block

Luca Barbato — 2013-05-17T21:15:36

Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC: libav-stable-LOOeJiBropLYtjvyW6yDsg< at >public.gmane.org
---
 libavcodec/wavpack.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
index 999e532..233c533 100644
--- a/libavcodec/wavpack.c
+++ b/libavcodec/wavpack.c
< at >< at > -795,6 +795,9 < at >< at > static int wavpack_decode_block(AVCodecContext *avctx, int block_no,

     if (!wc->mkv_mode) {
         s->samples = AV_RL32(buf); buf += 4;
+        if (s->samples > wc->samples)
+            return AVERROR_INVALIDDATA;
+
         if (!s->samples) {
             *got_frame_ptr = 0;
             return 0;
--
1.8.2.1

Re: [PATCH] build: do not leave asserts enabled

Luca Barbato — 2013-05-17T20:04:22

I consider this exactly an improper and dangerous use of assert().

If you know already that codepath would lead to a crash the proper
solution is to return an error to the caller.

Imagine having your browser/vnc-terminal/chat-application crash because
of a network glitch because you put an assert() in a decoding routine
(e.g. the vorbis one pointed by j-b).

The proper way is to return an error to the program and then the program
can decide what to do about it.

A library MUST not randomly crash, on purpose or not.

While developing sprinkling asserts so you can see clearly where your
assumptions fail is perfectly fine.

lu

Re: [PATCH] build: do not leave asserts enabled

Anton Khirnov — 2013-05-17T19:53:25

On Fri, 17 May 2013 21:16:12 +0200, Luca Barbato <lu_zero-aBrp7R+bbdUdnm+yROfE0A< at >public.gmane.org> wrote:

I disagree.
A proper use for an assert is when the code following it would crash anyway if
the assert wasn't there and the asserted condition was false. In such a case the
assert makes it much easier to track what went wrong. It can also simplify
understanding the author's intent.

Re: [PATCH] prores: decode alpha plane when it'spresent

Anton Khirnov — 2013-05-17T19:33:00

On Fri, 17 May 2013 21:17:04 +0200, Kostya Shishkov <kostya.shishkov-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:

I don't really see what's the advantage, but surewhynot.

Re: [PATCH] prores: decode alpha plane when it's present

Kostya Shishkov — 2013-05-17T19:17:04

Would you agree to this patch as is and the latter patch for prettifying it?

Re: [PATCH] build: do not leave asserts enabled

Luca Barbato — 2013-05-17T19:16:12

So only when developing something, not when releasing. Leaving active
asserts in release builds is horrible.


Some of them should be converted to proper checks with return values.

lu