[FFmpeg-devel] [PATCH 02/18] avcodec/vp8: Disable lf_delta for VP7

[Ronald S. Bultje]

Hi,

On Sun, Sep 11, 2022 at 10:41 AM Ronald S. Bultje <rsbultje at gmail.com>
wrote:

> Hi,
>
> On Sun, Sep 11, 2022 at 12:38 AM Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com> wrote:
>
>> Peter Ross:
>> > On Sat, Sep 10, 2022 at 03:07:13AM +0200, Andreas Rheinhardt wrote:
>> >> It is a VP8-only feature.
>> >>
>> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> >> ---
>> >>  libavcodec/vp8.c | 3 +--
>> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
>> >> index c00c5c975e..04a2113cc8 100644
>> >> --- a/libavcodec/vp8.c
>> >> +++ b/libavcodec/vp8.c
>> >> @@ -637,7 +637,6 @@ static int vp7_decode_frame_header(VP8Context *s,
>> const uint8_t *buf, int buf_si
>> >>          for (i = 0; i < 2; i++)
>> >>              memcpy(s->prob->mvc[i], vp7_mv_default_prob[i],
>> >>                     sizeof(vp7_mv_default_prob[i]));
>> >> -        memset(&s->lf_delta, 0, sizeof(s->lf_delta));
>> >>          memcpy(s->prob[0].scan, ff_zigzag_scan,
>> sizeof(s->prob[0].scan));
>> >>      }
>> >>
>> >> @@ -2171,7 +2170,7 @@ void filter_level_for_mb(VP8Context *s,
>> VP8Macroblock *mb,
>> >>      } else
>> >>          filter_level = s->filter.level;
>> >>
>> >> -    if (s->lf_delta.enabled) {
>> >> +    if (!is_vp7 && s->lf_delta.enabled) {
>> >>          filter_level += s->lf_delta.ref[mb->ref_frame];
>> >>          filter_level += s->lf_delta.mode[mb->mode];
>> >>      }
>> >
>> > what's the motivation for patches 01 and 02?
>> > are you not just adding another condition (is_vp7) to evaluate at
>> decode time?
>> > if its improved readability, then suggest renaming 'lf_delta' to
>> 'lf_delta_vp8'
>> >
>> > pathces 3-18 look fine to me.
>> >
>>
>> is_vp7 is evaluated at compile-time
>>
>
> This should probably be changed to be uppercase then. Lowercase suggests
> it's a variable.
>

It's inline, not macro, apparently.

I don't like the impact on readability here. Part of me wonders whether it
doesn't make more sense to split this file in vp7.c, vp8.c and vp78.c, and
have a proper design of two decoders and separate/shared parent/child
contexts definitions... A classic FFmpeg dilemma, I guess. The problem is
basically that this complicates people who have to debug this code when
issues occur (me) at the benefit of losing some dead code in vp7. I'm not
super-excited about that...

Ronald