Re: [FFmpeg-devel] [PATCH 02/18] avcodec/vp8: Disable lf_delta for VP7
On Sun, Sep 11, 2022 at 06:38:39AM +0200, Andreas Rheinhardt 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
> >> ---
> >> 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, because all the functions that use
> is_vp7 in this decoder are marked as av_always_inline.
> If it were otherwise, several of the patches 3-18 would make no sense
> and would just add another runtime check.
ok make sense
-- 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] [PATCH 02/18] avcodec/vp8: Disable lf_delta for VP7
Hi,
On Sun, Sep 11, 2022 at 10:41 AM Ronald S. Bultje
wrote:
> Hi,
>
> On Sun, Sep 11, 2022 at 12:38 AM Andreas Rheinhardt <
> andreas.rheinha...@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
>> >> ---
>> >> 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
___
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] [PATCH 02/18] avcodec/vp8: Disable lf_delta for VP7
Hi,
On Sun, Sep 11, 2022 at 12:38 AM Andreas Rheinhardt <
andreas.rheinha...@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
> >> ---
> >> 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.
Ronald
___
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] [PATCH 02/18] avcodec/vp8: Disable lf_delta for VP7
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
>> ---
>> 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, because all the functions that use
is_vp7 in this decoder are marked as av_always_inline.
If it were otherwise, several of the patches 3-18 would make no sense
and would just add another runtime check.
- Andreas
___
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] [PATCH 02/18] avcodec/vp8: Disable lf_delta for VP7
On Sat, Sep 10, 2022 at 03:07:13AM +0200, Andreas Rheinhardt wrote:
> It is a VP8-only feature.
>
> Signed-off-by: Andreas Rheinhardt
> ---
> 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.
-- 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".
