Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-23 Thread Peter Ross 
On Tue, May 21, 2019 at 09:30:54PM +0200, Reimar Döffinger wrote:
> On Tue, May 21, 2019 at 05:44:20PM +1000, Peter Ross wrote:
> > +if (bits < 0x100) {
> > +skip_bits(gb, 1);
> > +} else if (bits < 0x180) {
> > +skip_bits(gb, 2);
> > +v += 1;
> > +}
> > +#define body(n) { \
> > +skip_bits(gb, 2 + n); \
> > +v += (1 << n) + get_bits(gb, n); }
> > +#define else_if(thresh, n) else if (bits < thresh) body(n)
> 
> Not sure I think the defines are great for readability,
> but if you want to fully encode the logic, you could go for
> e.g.
> #define else_if(n) else if (bits < (0x200 - (0x80 >> n))) body(n)
> Also as to the earlier discussed early bailout for the +257 case:
> it seems sensible values can't really be larger than yuv_macroblock_count
> and I think FFmpeg has defines for maximum frame width/height that
> you could thus use to have a non-arbitrary bailout value?

discussed in other email.

> > +has_partial = 0;
> > +bit = get_bits1(gb);
> > +current_run = vp4_read_mb_value(gb);
> > +
> > +for (i = 0; i < s->yuv_macroblock_count; i++) {
> > +if (!current_run) {
> > +bit ^= 1;
> > +current_run = vp4_read_mb_value(gb);
> > +}
> > +s->superblock_coding[i] = 2 * bit;
> > +has_partial |= bit == 0;
> > +current_run--;
> > +}
> 
> This code doesn't quite look right to me.
> For both of the vp4_read_mb_value weird stuff
> seems to happen when it returns 0,
i in the first case it directly flips bit
> and reads a new value, which is stupid wasteful
> encoding, in the second case current_run underflows
> on current_run--, which is undefined behaviour
> - or at least weird?

not quite that wasteful. vp4_read_mb_value() always returns >0.
the ==0 test occurs only when the current_run counter is exahusted, and needs 
to be
reloaded with vp4_read_mb_value().
with your optimisation below the ==0 test is not needed.

> Except for that, isn't that the same as
>bit = get_bits1(gb);
>for (i = 0; i < s->yuv_macroblock_count; ) {
>current_run = vp4_read_mb_value(gb);
>if (current_run > s->yuv_macroblock_count - i) -> report error?
>if (current_run == 0) current_run = s->yuv_macroblock_count - i; // 
> maybe??
>memset(s->superblock_coding + i, 2*bit, current_run);
>has_partial |= bit == 0;
>i += current_run;
>bit ^= 1;
>}

this is far nicer. i will squish the i += part into the for loop.

> Hm, admittedly this doesn't really make much
> sense as you can't apply this trick to the has_partial
> case.
> But still maybe the current_run too large and 0
> cases could be clarified at least.

done.

> > +int mb_y = 2 * sb_y + (((j >> 1) + j) & 1);
> 
> Is ^ potentially clearer than + here?

yes:  int mb_y = 2 * sb_y + (j >> 1) ^ (j & 1);


> > +for (k = 0; k < 4; k++) {
> > +if (BLOCK_X >= fragment_width || BLOCK_Y >= 
> > fragment_height)
> > +continue;
> > +fragment = s->fragment_start[plane] + BLOCK_Y * 
> > fragment_width + BLOCK_X;
> > +
> > +coded = pattern & (1 << (3 - k));
> 
> coded = pattern & (8 >> k);
> maybe?
> 
> > +if (last_motion < 0)
> > +v = -v;
> > +return v;
> 
> I'd probably be partial to using ?: here, but your decision.

agree. leftover from when i had #ifdef trace line everywhere.

> > +if (coding_mode == 2) { /* VP4 */
> > +motion_x[0] = vp4_get_mv(s, gb, 0, 
> > last_gold_motion_x);
> > +motion_y[0] = vp4_get_mv(s, gb, 1, 
> > last_gold_motion_y);
> > +last_gold_motion_x = motion_x[0];
> > +last_gold_motion_y = motion_y[0];
> 
> Could write as
> last_gold_motion_x =
> motion_x[0] = vp4_get_mv(s, gb, 0, last_gold_motion_x);
> I think?
> But no strong opinion either.

agree.

> > @@ -1012,8 +1214,8 @@ static int unpack_vlcs(Vp3DecodeContext *s, 
> > GetBitContext *gb,
> >  bits_to_get = coeff_get_bits[token];
> >  if (bits_to_get)
> >  bits_to_get = get_bits(gb, bits_to_get);
> > -coeff = coeff_tables[token][bits_to_get];
> >
> > +coeff = coeff_tables[token][bits_to_get];
> 
> cosmetic?

yes.

> > +eob_run = eob_run_base[token];
> > +if (eob_run_get_bits[token])
> [...]
> > +zero_run = zero_run_base[token];
> > +if (zero_run_get_bits[token])
> 
> If _run_base and _run_get_bits are always used together like this,
> wouldn't it for readability and cache locality be better to
> make it an array of structs so they are next to each other in memory?

readability, yes. performance, maybe as those arrays are only seven bytes each.
this change also touches VP3/Theora. i want to benchmark it.

> > +

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-23 Thread James Almer 
On 5/23/2019 9:51 AM, Lynne wrote:
> May 23, 2019, 12:22 PM by pr...@xvid.org:
> 
>> On Tue, May 21, 2019 at 08:39:59PM +0200, Carl Eugen Hoyos wrote:
>>
>>> Am Di., 21. Mai 2019 um 19:18 Uhr schrieb Lynne :

 May 21, 2019, 8:44 AM by pr...@xvid.org :

> ---
>
> what's changed:
> * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
> * improved vp4_read_mb_value thanks to reminars suggestions
> * improved configure vp3_decoder_select
>
>
> Changelog   |1 +
> configure   |1 +
> doc/general.texi|2 +
> libavcodec/Makefile |1 +
> libavcodec/allcodecs.c  |1 +
> libavcodec/avcodec.h|1 +
> libavcodec/codec_desc.c |7 +
> libavcodec/vp3.c|  746 ++--
> libavcodec/vp4data.h| 1186 +++
> 9 files changed, 1911 insertions(+), 35 deletions(-)
> create mode 100644 libavcodec/vp4data.h
>

 Just remove CONFIG_VP4_DECODER and make it part of the vp3 decoder.
>>>
>>> Wasn't this explicitly requested in an earlier review?
>>> (And it is common within FFmpeg)
>>>
>>
>> i'll leave the ifdefs inplace.
>>
>> lynne has a point though, disabling vp4 only reduces the final ffmpeg binary 
>> by 25 kilobytes.
>>
> 
> Do remove them then, its not that much, regardless of what was said in a 
> previous review,
> even you think so.

No, if the vp4 decoder can't be merged into the vp3 one, then code
exclusive for it should be wrapped in preprocessor checks, same as we're
doing with Theora.

There's a reason libavcodec is so modularized.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-23 Thread Lynne 
May 23, 2019, 12:22 PM by pr...@xvid.org:

> On Tue, May 21, 2019 at 08:39:59PM +0200, Carl Eugen Hoyos wrote:
>
>> Am Di., 21. Mai 2019 um 19:18 Uhr schrieb Lynne :
>> >
>> > May 21, 2019, 8:44 AM by pr...@xvid.org :
>> >
>> > > ---
>> > >
>> > > what's changed:
>> > > * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
>> > > * improved vp4_read_mb_value thanks to reminars suggestions
>> > > * improved configure vp3_decoder_select
>> > >
>> > >
>> > > Changelog   |1 +
>> > > configure   |1 +
>> > > doc/general.texi|2 +
>> > > libavcodec/Makefile |1 +
>> > > libavcodec/allcodecs.c  |1 +
>> > > libavcodec/avcodec.h|1 +
>> > > libavcodec/codec_desc.c |7 +
>> > > libavcodec/vp3.c|  746 ++--
>> > > libavcodec/vp4data.h| 1186 +++
>> > > 9 files changed, 1911 insertions(+), 35 deletions(-)
>> > > create mode 100644 libavcodec/vp4data.h
>> > >
>> >
>> > Just remove CONFIG_VP4_DECODER and make it part of the vp3 decoder.
>>
>> Wasn't this explicitly requested in an earlier review?
>> (And it is common within FFmpeg)
>>
>
> i'll leave the ifdefs inplace.
>
> lynne has a point though, disabling vp4 only reduces the final ffmpeg binary 
> by 25 kilobytes.
>

Do remove them then, its not that much, regardless of what was said in a 
previous review,
even you think so.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-23 Thread Peter Ross 
On Tue, May 21, 2019 at 11:34:34AM +0200, Tomas Härdin wrote:
> tis 2019-05-21 klockan 17:44 +1000 skrev Peter Ross:
> > ---
> > 
> > what's changed:
> > * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
> > * improved vp4_read_mb_value thanks to reminars suggestions
> > * improved configure vp3_decoder_select

> > +#if CONFIG_VP4_DECODER
> > +if (s->version >= 2) {
> > +int mb_height, mb_width;
> > +int mb_width_mul, mb_width_div, mb_height_mul, 
> > mb_height_div;
> > +
> > +mb_height = get_bits(, 8);
> > +mb_width  = get_bits(, 8);
> > +if (mb_height != s->macroblock_height ||
> > +mb_width != s->macroblock_width)
> > +av_log(s->avctx, AV_LOG_WARNING, "VP4 header: Warning, 
> > macroblock dimension mismatch");
> > +
> > +mb_width_mul = get_bits(, 5);
> > +mb_width_div = get_bits(, 3);
> > +mb_height_mul = get_bits(, 5);
> > +mb_height_div = get_bits(, 3);
> > +if (mb_width_mul != 1 || mb_width_div != 1 || 
> > mb_height_mul != 1 || mb_height_div != 1)
> > +  av_log(s->avctx, AV_LOG_WARNING, "VP4 header: 
> > Warning, unexpected macroblock dimension multipler/divider");
> > +
> > +if (get_bits(, 2))
> > +av_log(s->avctx, AV_LOG_WARNING, "VP4 header: Warning, 
> > unknown bits set");
> 
> Maybe these should be errors and/or requests for samples? It macroblock
> count changes that may indicate a resolution change

agree. and no need for "VP4...:"  as it will print the context as vp4.

other items fixed. thanks.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-23 Thread Peter Ross 
On Wed, May 22, 2019 at 09:45:46PM +0200, Reimar Döffinger wrote:
> On Tue, May 21, 2019 at 09:30:54PM +0200, Reimar Döffinger wrote:
> > > +#define SHIFT(v, shift) ((v) >> (shift))
> > > +#define ABS_SHIFT(v, shift) ((v) > 0 ? SHIFT(v, shift) : -SHIFT(-v, 
> > > shift))
> >
> > Don't we have something like that already?
> 
> Seems we don't
> 
> > I think this should rather be:
> > (v - (v >> 31)) >> shift
> > ?
> 
> Not right for shift > 1.
> But this one according to my testing is equivalent:
> ((a + (a >> 31)) >> b) - (a >> 31)
> Not certain it will be fewer instructions,
> but it is branchless.

> On some ISAs the simple
> a / (1 << b)

this is best, and the compiler can decode. thanks.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-23 Thread Peter Ross 
On Tue, May 21, 2019 at 08:38:02PM +0200, Reimar Döffinger wrote:
> On Tue, May 21, 2019 at 11:15:03AM -0300, James Almer wrote:
> > > I have a feeling this loop should have a stop condition like v <
> > > SOME_LARGE_VALUE, say INT_MAX-255 or yuv_macroblock_count, to reject
> > > corrupt/malicious files and not cause undefined behavior
> >
> > Using get_bits_left(gb) would be better than an arbitrary large value.
> 
> It seems the original comment wasn't preserved, but get_bits_left
> is fairly pointless because the 0-padding will cause loop exit
> anyway.
> Also get_bits_left wouldn't address the point that a 2GB input frame
> of all-1s from the right position would here end up reading 2GB
> 9 bits at a time.
> Overflow by my calculations would only happen after > 500 GB,
> so not sure that's a worry.
> But depending on the contexts in which this function is used,
> there might be obvious limits for v, in which case an early
> exit would make sense (even when not, runs > 250 bytes can
> likely safely assumed broken).

x/y macroblock counts are limited 8-bit, so yuv_macroblock_count never
exceeds (255 * 255 * 3/2 = 97537). i this kind of check will suffice
and speed up fuzzing tests.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-23 Thread Peter Ross 
On Tue, May 21, 2019 at 08:42:17PM +0200, Carl Eugen Hoyos wrote:
> Am Di., 21. Mai 2019 um 09:45 Uhr schrieb Peter Ross :
> 
> > diff --git a/configure b/configure
> > index 9b4305cf0d..61eb774116 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2825,6 +2825,7 @@ vc1image_decoder_select="vc1_decoder"
> >  vorbis_decoder_select="mdct"
> >  vorbis_encoder_select="audio_frame_queue mdct"
> >  vp3_decoder_select="hpeldsp vp3dsp videodsp"
> > +vp4_decoder_select="vp3_decoder"
> >  vp5_decoder_select="h264chroma hpeldsp videodsp vp3dsp vp56dsp"
> >  vp6_decoder_select="h264chroma hpeldsp huffman videodsp vp3dsp vp56dsp"
> >  vp6a_decoder_select="vp6_decoder"
> 
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index edccd73037..d76f392f1e 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -663,6 +663,7 @@ OBJS-$(CONFIG_VORBIS_DECODER)  += vorbisdec.o 
> > vorbisdsp.o vorbis.o \
> >  OBJS-$(CONFIG_VORBIS_ENCODER)  += vorbisenc.o vorbis.o \
> >vorbis_data.o
> >  OBJS-$(CONFIG_VP3_DECODER) += vp3.o
> > +OBJS-$(CONFIG_VP4_DECODER) += vp3.o
> 
> Imo, exactly one of these hunks should be committed.

thanks.

> Do you know if the claim on Wikipedia that "VP4" was only an encoder
> (for VP3) was true for the actual sold software or if Wikipedia is just
> wrong?

VP4 was a "new" encoder and decoder. New in quotes because nothing is truely
new in this industry.

the VP4 software was initially not available to the public. eventually on2
provided a community version. there were VfW binaries. thre was also a
real-media player plugin dll, but unfortunately i have no content for it.

yes, the VP4 bitstream is similar to VP3, but so were the VP5/6 and
VP7/8 and VP9/10 bitstreams.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-23 Thread Peter Ross 
On Tue, May 21, 2019 at 08:39:59PM +0200, Carl Eugen Hoyos wrote:
> Am Di., 21. Mai 2019 um 19:18 Uhr schrieb Lynne :
> >
> > May 21, 2019, 8:44 AM by pr...@xvid.org :
> >
> > > ---
> > >
> > > what's changed:
> > > * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
> > > * improved vp4_read_mb_value thanks to reminars suggestions
> > > * improved configure vp3_decoder_select
> > >
> > >
> > > Changelog   |1 +
> > > configure   |1 +
> > > doc/general.texi|2 +
> > > libavcodec/Makefile |1 +
> > > libavcodec/allcodecs.c  |1 +
> > > libavcodec/avcodec.h|1 +
> > > libavcodec/codec_desc.c |7 +
> > > libavcodec/vp3.c|  746 ++--
> > > libavcodec/vp4data.h| 1186 +++
> > > 9 files changed, 1911 insertions(+), 35 deletions(-)
> > > create mode 100644 libavcodec/vp4data.h
> > >
> >
> > Just remove CONFIG_VP4_DECODER and make it part of the vp3 decoder.
> 
> Wasn't this explicitly requested in an earlier review?
> (And it is common within FFmpeg)

i'll leave the ifdefs inplace.

lynne has a point though, disabling vp4 only reduces the final ffmpeg binary by 
25 kilobytes.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-22 Thread Reimar Döffinger 
On Tue, May 21, 2019 at 09:30:54PM +0200, Reimar Döffinger wrote:
> > +#define SHIFT(v, shift) ((v) >> (shift))
> > +#define ABS_SHIFT(v, shift) ((v) > 0 ? SHIFT(v, shift) : -SHIFT(-v, shift))
>
> Don't we have something like that already?

Seems we don't

> I think this should rather be:
> (v - (v >> 31)) >> shift
> ?

Not right for shift > 1.
But this one according to my testing is equivalent:
((a + (a >> 31)) >> b) - (a >> 31)
Not certain it will be fewer instructions,
but it is branchless.
On some ISAs the simple
a / (1 << b)
might even be faster, so maybe worth
going for that...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-21 Thread James Almer 
On 5/21/2019 3:42 PM, Carl Eugen Hoyos wrote:
> Am Di., 21. Mai 2019 um 09:45 Uhr schrieb Peter Ross :
> 
>> diff --git a/configure b/configure
>> index 9b4305cf0d..61eb774116 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2825,6 +2825,7 @@ vc1image_decoder_select="vc1_decoder"
>>  vorbis_decoder_select="mdct"
>>  vorbis_encoder_select="audio_frame_queue mdct"
>>  vp3_decoder_select="hpeldsp vp3dsp videodsp"
>> +vp4_decoder_select="vp3_decoder"
>>  vp5_decoder_select="h264chroma hpeldsp videodsp vp3dsp vp56dsp"
>>  vp6_decoder_select="h264chroma hpeldsp huffman videodsp vp3dsp vp56dsp"
>>  vp6a_decoder_select="vp6_decoder"
> 
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index edccd73037..d76f392f1e 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -663,6 +663,7 @@ OBJS-$(CONFIG_VORBIS_DECODER)  += vorbisdec.o 
>> vorbisdsp.o vorbis.o \
>>  OBJS-$(CONFIG_VORBIS_ENCODER)  += vorbisenc.o vorbis.o \
>>vorbis_data.o
>>  OBJS-$(CONFIG_VP3_DECODER) += vp3.o
>> +OBJS-$(CONFIG_VP4_DECODER) += vp3.o
> 
> Imo, exactly one of these hunks should be committed.

The configure one. It would then be the same as with the Theora decoder.

> 
> Do you know if the claim on Wikipedia that "VP4" was only an encoder
> (for VP3) was true for the actual sold software or if Wikipedia is just
> wrong?
> 
> Thank you, Carl Eugen
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-21 Thread Reimar Döffinger 
On Tue, May 21, 2019 at 05:44:20PM +1000, Peter Ross wrote:
> +if (bits < 0x100) {
> +skip_bits(gb, 1);
> +} else if (bits < 0x180) {
> +skip_bits(gb, 2);
> +v += 1;
> +}
> +#define body(n) { \
> +skip_bits(gb, 2 + n); \
> +v += (1 << n) + get_bits(gb, n); }
> +#define else_if(thresh, n) else if (bits < thresh) body(n)

Not sure I think the defines are great for readability,
but if you want to fully encode the logic, you could go for
e.g.
#define else_if(n) else if (bits < (0x200 - (0x80 >> n))) body(n)
Also as to the earlier discussed early bailout for the +257 case:
it seems sensible values can't really be larger than yuv_macroblock_count
and I think FFmpeg has defines for maximum frame width/height that
you could thus use to have a non-arbitrary bailout value?

> +has_partial = 0;
> +bit = get_bits1(gb);
> +current_run = vp4_read_mb_value(gb);
> +
> +for (i = 0; i < s->yuv_macroblock_count; i++) {
> +if (!current_run) {
> +bit ^= 1;
> +current_run = vp4_read_mb_value(gb);
> +}
> +s->superblock_coding[i] = 2 * bit;
> +has_partial |= bit == 0;
> +current_run--;
> +}

This code doesn't quite look right to me.
For both of the vp4_read_mb_value weird stuff
seems to happen when it returns 0,
in the first case it directly flips bit
and reads a new value, which is stupid wasteful
encoding, in the second case current_run underflows
on current_run--, which is undefined behaviour
- or at least weird?

Except for that, isn't that the same as
   bit = get_bits1(gb);
   for (i = 0; i < s->yuv_macroblock_count; ) {
   current_run = vp4_read_mb_value(gb);
   if (current_run > s->yuv_macroblock_count - i) -> report error?
   if (current_run == 0) current_run = s->yuv_macroblock_count - i; // 
maybe??
   memset(s->superblock_coding + i, 2*bit, current_run);
   has_partial |= bit == 0;
   i += current_run;
   bit ^= 1;
   }

Hm, admittedly this doesn't really make much
sense as you can't apply this trick to the has_partial
case.
But still maybe the current_run too large and 0
cases could be clarified at least.

> +int mb_y = 2 * sb_y + (((j >> 1) + j) & 1);

Is ^ potentially clearer than + here?

> +for (k = 0; k < 4; k++) {
> +if (BLOCK_X >= fragment_width || BLOCK_Y >= 
> fragment_height)
> +continue;
> +fragment = s->fragment_start[plane] + BLOCK_Y * 
> fragment_width + BLOCK_X;
> +
> +coded = pattern & (1 << (3 - k));

coded = pattern & (8 >> k);
maybe?

> +if (last_motion < 0)
> +v = -v;
> +return v;

I'd probably be partial to using ?: here, but your decision.

> +if (coding_mode == 2) { /* VP4 */
> +motion_x[0] = vp4_get_mv(s, gb, 0, 
> last_gold_motion_x);
> +motion_y[0] = vp4_get_mv(s, gb, 1, 
> last_gold_motion_y);
> +last_gold_motion_x = motion_x[0];
> +last_gold_motion_y = motion_y[0];

Could write as
last_gold_motion_x =
motion_x[0] = vp4_get_mv(s, gb, 0, last_gold_motion_x);
I think?
But no strong opinion either.

> @@ -1012,8 +1214,8 @@ static int unpack_vlcs(Vp3DecodeContext *s, 
> GetBitContext *gb,
>  bits_to_get = coeff_get_bits[token];
>  if (bits_to_get)
>  bits_to_get = get_bits(gb, bits_to_get);
> -coeff = coeff_tables[token][bits_to_get];
>
> +coeff = coeff_tables[token][bits_to_get];

cosmetic?

> +eob_run = eob_run_base[token];
> +if (eob_run_get_bits[token])
[...]
> +zero_run = zero_run_base[token];
> +if (zero_run_get_bits[token])

If _run_base and _run_get_bits are always used together like this,
wouldn't it for readability and cache locality be better to
make it an array of structs so they are next to each other in memory?

> +vp4_dc_predictor_reset(_pred[j * 6 + i + 7]);
> +s->dc_pred_row[sb_x * 4 + i] = dc_pred[25 + i];
> +dc_pred[6 * i] = dc_pred[6 * i + 4];

If there's an easy way to make those constants like 6, 7 and 25
more obvious that might be a good idea.

> +if (dc_pred[idx - 6].type == type) {
> +dc += dc_pred[idx - 6].dc;
> +count++;
> +}
> +
> +if (dc_pred[idx + 6].type == type) {
> +dc += dc_pred[idx + 6].dc;
> +count++;
> +}
> +
> +if (count != 2 && dc_pred[idx - 1].type == type) {
> +dc += dc_pred[idx - 1].dc;
> +count++;
> +}
> +
> +if (count != 2 && dc_pred[idx + 1].type == type) {
> +dc += dc_pred[idx + 1].dc;
> +count++;
> +}

Maybe do dc_pred += idx at the start and then
only dc_pred[-6], dc_pred[6] etc?

> +#define loop_stride 12
> +uint8_t loop[12 * loop_stride];

Hm, at 144 bytes, might it make

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-21 Thread Reimar Döffinger 
On Tue, May 21, 2019 at 11:15:03AM -0300, James Almer wrote:
> > I have a feeling this loop should have a stop condition like v <
> > SOME_LARGE_VALUE, say INT_MAX-255 or yuv_macroblock_count, to reject
> > corrupt/malicious files and not cause undefined behavior
>
> Using get_bits_left(gb) would be better than an arbitrary large value.

It seems the original comment wasn't preserved, but get_bits_left
is fairly pointless because the 0-padding will cause loop exit
anyway.
Also get_bits_left wouldn't address the point that a 2GB input frame
of all-1s from the right position would here end up reading 2GB
9 bits at a time.
Overflow by my calculations would only happen after > 500 GB,
so not sure that's a worry.
But depending on the contexts in which this function is used,
there might be obvious limits for v, in which case an early
exit would make sense (even when not, runs > 250 bytes can
likely safely assumed broken).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-21 Thread Carl Eugen Hoyos 
Am Di., 21. Mai 2019 um 09:45 Uhr schrieb Peter Ross :

> diff --git a/configure b/configure
> index 9b4305cf0d..61eb774116 100755
> --- a/configure
> +++ b/configure
> @@ -2825,6 +2825,7 @@ vc1image_decoder_select="vc1_decoder"
>  vorbis_decoder_select="mdct"
>  vorbis_encoder_select="audio_frame_queue mdct"
>  vp3_decoder_select="hpeldsp vp3dsp videodsp"
> +vp4_decoder_select="vp3_decoder"
>  vp5_decoder_select="h264chroma hpeldsp videodsp vp3dsp vp56dsp"
>  vp6_decoder_select="h264chroma hpeldsp huffman videodsp vp3dsp vp56dsp"
>  vp6a_decoder_select="vp6_decoder"

> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index edccd73037..d76f392f1e 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -663,6 +663,7 @@ OBJS-$(CONFIG_VORBIS_DECODER)  += vorbisdec.o 
> vorbisdsp.o vorbis.o \
>  OBJS-$(CONFIG_VORBIS_ENCODER)  += vorbisenc.o vorbis.o \
>vorbis_data.o
>  OBJS-$(CONFIG_VP3_DECODER) += vp3.o
> +OBJS-$(CONFIG_VP4_DECODER) += vp3.o

Imo, exactly one of these hunks should be committed.

Do you know if the claim on Wikipedia that "VP4" was only an encoder
(for VP3) was true for the actual sold software or if Wikipedia is just
wrong?

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-21 Thread Carl Eugen Hoyos 
Am Di., 21. Mai 2019 um 19:18 Uhr schrieb Lynne :
>
> May 21, 2019, 8:44 AM by pr...@xvid.org :
>
> > ---
> >
> > what's changed:
> > * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
> > * improved vp4_read_mb_value thanks to reminars suggestions
> > * improved configure vp3_decoder_select
> >
> >
> > Changelog   |1 +
> > configure   |1 +
> > doc/general.texi|2 +
> > libavcodec/Makefile |1 +
> > libavcodec/allcodecs.c  |1 +
> > libavcodec/avcodec.h|1 +
> > libavcodec/codec_desc.c |7 +
> > libavcodec/vp3.c|  746 ++--
> > libavcodec/vp4data.h| 1186 +++
> > 9 files changed, 1911 insertions(+), 35 deletions(-)
> > create mode 100644 libavcodec/vp4data.h
> >
>
> Just remove CONFIG_VP4_DECODER and make it part of the vp3 decoder.

Wasn't this explicitly requested in an earlier review?
(And it is common within FFmpeg)

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-21 Thread Lynne 
May 21, 2019, 8:44 AM by pr...@xvid.org :

> ---
>
> what's changed:
> * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
> * improved vp4_read_mb_value thanks to reminars suggestions
> * improved configure vp3_decoder_select
>
>
> Changelog   |1 +
> configure   |1 +
> doc/general.texi|2 +
> libavcodec/Makefile |1 +
> libavcodec/allcodecs.c  |1 +
> libavcodec/avcodec.h|1 +
> libavcodec/codec_desc.c |7 +
> libavcodec/vp3.c|  746 ++--
> libavcodec/vp4data.h| 1186 +++
> 9 files changed, 1911 insertions(+), 35 deletions(-)
> create mode 100644 libavcodec/vp4data.h
>

Just remove CONFIG_VP4_DECODER and make it part of the vp3 decoder.
Its unnecessary and the tables aren't big at all.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-21 Thread James Almer 
On 5/21/2019 6:34 AM, Tomas Härdin wrote:
> tis 2019-05-21 klockan 17:44 +1000 skrev Peter Ross:
>> ---
>>
>> what's changed:
>> * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
>> * improved vp4_read_mb_value thanks to reminars suggestions
>> * improved configure vp3_decoder_select
>>
>> [...]
>>  
>> +#define BLOCK_X (2 * mb_x + (k & 1))
>> +#define BLOCK_Y (2 * mb_y + (k >> 1))
>> +
>> +#if CONFIG_VP4_DECODER
>> +static int vp4_read_mb_value(GetBitContext *gb)
>> +{
>> +int v = 1;
>> +int bits = show_bits(gb, 9);
> 
> This call to show_bits() looks unnecessary
> 
>> +while ((bits = show_bits(gb, 9)) == 0x1ff) {
>> +skip_bits(gb, 9);
>> +v += 256;
>> +}
> 
> I have a feeling this loop should have a stop condition like v <
> SOME_LARGE_VALUE, say INT_MAX-255 or yuv_macroblock_count, to reject
> corrupt/malicious files and not cause undefined behavior

Using get_bits_left(gb) would be better than an arbitrary large value.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv4] VP4 video decoder

2019-05-21 Thread Tomas Härdin 
tis 2019-05-21 klockan 17:44 +1000 skrev Peter Ross:
> ---
> 
> what's changed:
> * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
> * improved vp4_read_mb_value thanks to reminars suggestions
> * improved configure vp3_decoder_select
> 
> [...]
>  
> +#define BLOCK_X (2 * mb_x + (k & 1))
> +#define BLOCK_Y (2 * mb_y + (k >> 1))
> +
> +#if CONFIG_VP4_DECODER
> +static int vp4_read_mb_value(GetBitContext *gb)
> +{
> +int v = 1;
> +int bits = show_bits(gb, 9);

This call to show_bits() looks unnecessary

> +while ((bits = show_bits(gb, 9)) == 0x1ff) {
> +skip_bits(gb, 9);
> +v += 256;
> +}

I have a feeling this loop should have a stop condition like v <
SOME_LARGE_VALUE, say INT_MAX-255 or yuv_macroblock_count, to reject
corrupt/malicious files and not cause undefined behavior

> +if (bits < 0x100) {
> +skip_bits(gb, 1);
> +} else if (bits < 0x180) {
> +skip_bits(gb, 2);
> +v += 1;
> +}
> +#define body(n) { \
> +skip_bits(gb, 2 + n); \
> +v += (1 << n) + get_bits(gb, n); }
> +#define else_if(thresh, n) else if (bits < thresh) body(n)
> +else_if(0x1c0, 1)
> +else_if(0x1e0, 2)
> +else_if(0x1f0, 3)
> +else_if(0x1f8, 4)
> +else_if(0x1fc, 5)
> +else_if(0x1fe, 6)
> +else body(7)
> +#undef body
> +#undef else_if
> +return v;
> +}

> @@ -1012,8 +1214,8 @@ static int unpack_vlcs(Vp3DecodeContext *s, 
> GetBitContext *gb,
>  bits_to_get = coeff_get_bits[token];
>  if (bits_to_get)
>  bits_to_get = get_bits(gb, bits_to_get);
> -coeff = coeff_tables[token][bits_to_get];
>  
> +coeff = coeff_tables[token][bits_to_get];

Stray hunk

> +#if CONFIG_VP4_DECODER
> +if (s->version >= 2) {
> +int mb_height, mb_width;
> +int mb_width_mul, mb_width_div, mb_height_mul, mb_height_div;
> +
> +mb_height = get_bits(, 8);
> +mb_width  = get_bits(, 8);
> +if (mb_height != s->macroblock_height ||
> +mb_width != s->macroblock_width)
> +av_log(s->avctx, AV_LOG_WARNING, "VP4 header: Warning, 
> macroblock dimension mismatch");
> +
> +mb_width_mul = get_bits(, 5);
> +mb_width_div = get_bits(, 3);
> +mb_height_mul = get_bits(, 5);
> +mb_height_div = get_bits(, 3);
> +if (mb_width_mul != 1 || mb_width_div != 1 || mb_height_mul 
> != 1 || mb_height_div != 1)
> +  av_log(s->avctx, AV_LOG_WARNING, "VP4 header: Warning, 
> unexpected macroblock dimension multipler/divider");
> +
> +if (get_bits(, 2))
> +av_log(s->avctx, AV_LOG_WARNING, "VP4 header: Warning, 
> unknown bits set");

Maybe these should be errors and/or requests for samples? It macroblock
count changes that may indicate a resolution change

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".