| Message ID | AS8P250MB07447F1CAE43BAE95279EC508F429@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/18] avcodec/vp8: Disable segmentation for VP7 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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@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. -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
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@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, 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
Hi, On Sun, Sep 11, 2022 at 12:38 AM Andreas Rheinhardt < andreas.rheinhardt@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@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. Ronald
Hi, On Sun, Sep 11, 2022 at 10:41 AM Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Sun, Sep 11, 2022 at 12:38 AM Andreas Rheinhardt < > andreas.rheinhardt@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@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
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 <andreas.rheinhardt@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, 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)
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]; }
It is a VP8-only feature. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/vp8.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)