Message ID | 5674DDE0-D4B3-4F88-B889-28A02ED9FCA4@amazon.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/3] - libavcodec/aom_film_grain: Handle byte alignment when multiple film grain parameters sets are present in the bitstream | expand |
Context | Check | Description |
---|---|---|
yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
andriy/configure_x86 | warning | Failed to apply patch |
On 20/09/2024 02:44, Segall, Andrew via ffmpeg-devel wrote: > Details: Add support for the apply_grain_flag. This allows the film grain process to be enabled/disabled for different display properties. > > On 9/8/24, 12:06 AM, "Andrew Segall" <asegall@amazon.com <mailto:asegall@amazon.com>> wrote: > > > Signed-off-by: Andrew Segall <asegall@amazon.com <mailto:asegall@amazon.com>> > --- > libavcodec/aom_film_grain.c | 6 ++++-- > libavcodec/hevc/hevcdec.c | 5 ++++- > libavfilter/vf_showinfo.c | 1 + > libavutil/film_grain_params.h | 5 +++++ > 4 files changed, 14 insertions(+), 3 deletions(-) > > > diff --git a/libavcodec/aom_film_grain.c b/libavcodec/aom_film_grain.c > index fdfa3dbf77..251a2793ac 100644 > --- a/libavcodec/aom_film_grain.c > +++ b/libavcodec/aom_film_grain.c > @@ -149,8 +149,10 @@ int ff_aom_parse_film_grain_sets(AVFilmGrainAFGS1Params *s, > fgp = &s->sets[set_idx]; > aom = &fgp->codec.aom; > > > - fgp->type = get_bits1(gb) ? AV_FILM_GRAIN_PARAMS_AV1 : AV_FILM_GRAIN_PARAMS_NONE; > - if (!fgp->type) > + fgp->type = AV_FILM_GRAIN_PARAMS_AV1; > + > + fgp->apply_grain = get_bits1(gb); > + if ( !fgp->apply_grain) > { > payload_bits = get_bits_count(gb) - start_position; > if (payload_bits > payload_size * 8) > diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c > index d915d74d22..8dd4fb10c5 100644 > --- a/libavcodec/hevc/hevcdec.c > +++ b/libavcodec/hevc/hevcdec.c > @@ -3155,7 +3155,10 @@ static int hevc_frame_end(HEVCContext *s) > &s->h274db, fgp); > break; > case AV_FILM_GRAIN_PARAMS_AV1: > - ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); > + if(fgp->apply_grain) > + ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); > + else > + out->needs_fg = 0; > break; > } > av_assert1(ret >= 0); > diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c > index f81df9d1bf..08e144ca46 100644 > --- a/libavfilter/vf_showinfo.c > +++ b/libavfilter/vf_showinfo.c > @@ -455,6 +455,7 @@ static void dump_sei_film_grain_params_metadata(AVFilterContext *ctx, const AVFr > } > > > av_log(ctx, AV_LOG_INFO, "type %s; ", film_grain_type_names[fgp->type]); > + av_log(ctx, AV_LOG_INFO, "apply_grain=%d; ", fgp->apply_grain); > av_log(ctx, AV_LOG_INFO, "seed=%"PRIu64"; ", fgp->seed); > av_log(ctx, AV_LOG_INFO, "width=%d; ", fgp->width); > av_log(ctx, AV_LOG_INFO, "height=%d; ", fgp->height); > diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h > index ccacab88fe..f3275923e1 100644 > --- a/libavutil/film_grain_params.h > +++ b/libavutil/film_grain_params.h > @@ -241,6 +241,11 @@ typedef struct AVFilmGrainParams { > */ > enum AVFilmGrainParamsType type; > > > + /** > + * Specifies if film grain parameters are enabled. > + */ > + int apply_grain; > + > /** > * Seed to use for the synthesis process, if the codec allows for it. > * This is an ABI break. Furthermore, what exactly does this improve over simply stripping the side data?
On 9/20/24, 12:15 AM, "ffmpeg-devel on behalf of Lynne via ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org <mailto:ffmpeg-devel-bounces@ffmpeg.org> on behalf of ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>> wrote: CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. On 20/09/2024 02:44, Segall, Andrew via ffmpeg-devel wrote: > Details: Add support for the apply_grain_flag. This allows the film grain process to be enabled/disabled for different display properties. > > On 9/8/24, 12:06 AM, "Andrew Segall" <asegall@amazon.com <mailto:asegall@amazon.com> <mailto:asegall@amazon.com <mailto:asegall@amazon.com>>> wrote: > > > Signed-off-by: Andrew Segall <asegall@amazon.com <mailto:asegall@amazon.com> <mailto:asegall@amazon.com <mailto:asegall@amazon.com>>> > --- > libavcodec/aom_film_grain.c | 6 ++++-- > libavcodec/hevc/hevcdec.c | 5 ++++- > libavfilter/vf_showinfo.c | 1 + > libavutil/film_grain_params.h | 5 +++++ > 4 files changed, 14 insertions(+), 3 deletions(-) > > > diff --git a/libavcodec/aom_film_grain.c b/libavcodec/aom_film_grain.c > index fdfa3dbf77..251a2793ac 100644 > --- a/libavcodec/aom_film_grain.c > +++ b/libavcodec/aom_film_grain.c > @@ -149,8 +149,10 @@ int ff_aom_parse_film_grain_sets(AVFilmGrainAFGS1Params *s, > fgp = &s->sets[set_idx]; > aom = &fgp->codec.aom; > > > - fgp->type = get_bits1(gb) ? AV_FILM_GRAIN_PARAMS_AV1 : AV_FILM_GRAIN_PARAMS_NONE; > - if (!fgp->type) > + fgp->type = AV_FILM_GRAIN_PARAMS_AV1; > + > + fgp->apply_grain = get_bits1(gb); > + if ( !fgp->apply_grain) > { > payload_bits = get_bits_count(gb) - start_position; > if (payload_bits > payload_size * 8) > diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c > index d915d74d22..8dd4fb10c5 100644 > --- a/libavcodec/hevc/hevcdec.c > +++ b/libavcodec/hevc/hevcdec.c > @@ -3155,7 +3155,10 @@ static int hevc_frame_end(HEVCContext *s) > &s->h274db, fgp); > break; > case AV_FILM_GRAIN_PARAMS_AV1: > - ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); > + if(fgp->apply_grain) > + ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); > + else > + out->needs_fg = 0; > break; > } > av_assert1(ret >= 0); > diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c > index f81df9d1bf..08e144ca46 100644 > --- a/libavfilter/vf_showinfo.c > +++ b/libavfilter/vf_showinfo.c > @@ -455,6 +455,7 @@ static void dump_sei_film_grain_params_metadata(AVFilterContext *ctx, const AVFr > } > > > av_log(ctx, AV_LOG_INFO, "type %s; ", film_grain_type_names[fgp->type]); > + av_log(ctx, AV_LOG_INFO, "apply_grain=%d; ", fgp->apply_grain); > av_log(ctx, AV_LOG_INFO, "seed=%"PRIu64"; ", fgp->seed); > av_log(ctx, AV_LOG_INFO, "width=%d; ", fgp->width); > av_log(ctx, AV_LOG_INFO, "height=%d; ", fgp->height); > diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h > index ccacab88fe..f3275923e1 100644 > --- a/libavutil/film_grain_params.h > +++ b/libavutil/film_grain_params.h > @@ -241,6 +241,11 @@ typedef struct AVFilmGrainParams { > */ > enum AVFilmGrainParamsType type; > > > + /** > + * Specifies if film grain parameters are enabled. > + */ > + int apply_grain; > + > /** > * Seed to use for the synthesis process, if the codec allows for it. > * > This is an ABI break. > Furthermore, what exactly does this improve over simply stripping the side data? Mainly addressing conformance issues. Example is that we have two parameters in the AFGS1 message. The first is associated with resolution A and apply_grain=1. Second is associated with resolution B and apply_grain=0. If the synthesis Is performed at resolution B, then the message says that we shouldn't perform film grain synthesis since apply_grain=0. However, if we strip, the information is lost - and synthesis may use the parameters associated with resolution A instead.
On 20/09/2024 09:42, Segall, Andrew wrote: > > > On 9/20/24, 12:15 AM, "ffmpeg-devel on behalf of Lynne via ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org <mailto:ffmpeg-devel-bounces@ffmpeg.org> on behalf of ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>> wrote: > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > > > > On 20/09/2024 02:44, Segall, Andrew via ffmpeg-devel wrote: >> Details: Add support for the apply_grain_flag. This allows the film grain process to be enabled/disabled for different display properties. >> >> On 9/8/24, 12:06 AM, "Andrew Segall" <asegall@amazon.com <mailto:asegall@amazon.com> <mailto:asegall@amazon.com <mailto:asegall@amazon.com>>> wrote: >> >> >> Signed-off-by: Andrew Segall <asegall@amazon.com <mailto:asegall@amazon.com> <mailto:asegall@amazon.com <mailto:asegall@amazon.com>>> >> --- >> libavcodec/aom_film_grain.c | 6 ++++-- >> libavcodec/hevc/hevcdec.c | 5 ++++- >> libavfilter/vf_showinfo.c | 1 + >> libavutil/film_grain_params.h | 5 +++++ >> 4 files changed, 14 insertions(+), 3 deletions(-) >> >> >> diff --git a/libavcodec/aom_film_grain.c b/libavcodec/aom_film_grain.c >> index fdfa3dbf77..251a2793ac 100644 >> --- a/libavcodec/aom_film_grain.c >> +++ b/libavcodec/aom_film_grain.c >> @@ -149,8 +149,10 @@ int ff_aom_parse_film_grain_sets(AVFilmGrainAFGS1Params *s, >> fgp = &s->sets[set_idx]; >> aom = &fgp->codec.aom; >> >> >> - fgp->type = get_bits1(gb) ? AV_FILM_GRAIN_PARAMS_AV1 : AV_FILM_GRAIN_PARAMS_NONE; >> - if (!fgp->type) >> + fgp->type = AV_FILM_GRAIN_PARAMS_AV1; >> + >> + fgp->apply_grain = get_bits1(gb); >> + if ( !fgp->apply_grain) >> { >> payload_bits = get_bits_count(gb) - start_position; >> if (payload_bits > payload_size * 8) >> diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c >> index d915d74d22..8dd4fb10c5 100644 >> --- a/libavcodec/hevc/hevcdec.c >> +++ b/libavcodec/hevc/hevcdec.c >> @@ -3155,7 +3155,10 @@ static int hevc_frame_end(HEVCContext *s) >> &s->h274db, fgp); >> break; >> case AV_FILM_GRAIN_PARAMS_AV1: >> - ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); >> + if(fgp->apply_grain) >> + ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); >> + else >> + out->needs_fg = 0; >> break; >> } >> av_assert1(ret >= 0); >> diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c >> index f81df9d1bf..08e144ca46 100644 >> --- a/libavfilter/vf_showinfo.c >> +++ b/libavfilter/vf_showinfo.c >> @@ -455,6 +455,7 @@ static void dump_sei_film_grain_params_metadata(AVFilterContext *ctx, const AVFr >> } >> >> >> av_log(ctx, AV_LOG_INFO, "type %s; ", film_grain_type_names[fgp->type]); >> + av_log(ctx, AV_LOG_INFO, "apply_grain=%d; ", fgp->apply_grain); >> av_log(ctx, AV_LOG_INFO, "seed=%"PRIu64"; ", fgp->seed); >> av_log(ctx, AV_LOG_INFO, "width=%d; ", fgp->width); >> av_log(ctx, AV_LOG_INFO, "height=%d; ", fgp->height); >> diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h >> index ccacab88fe..f3275923e1 100644 >> --- a/libavutil/film_grain_params.h >> +++ b/libavutil/film_grain_params.h >> @@ -241,6 +241,11 @@ typedef struct AVFilmGrainParams { >> */ >> enum AVFilmGrainParamsType type; >> >> >> + /** >> + * Specifies if film grain parameters are enabled. >> + */ >> + int apply_grain; >> + >> /** >> * Seed to use for the synthesis process, if the codec allows for it. >> * > > >> This is an ABI break. > >> Furthermore, what exactly does this improve over simply stripping the side data? > > Mainly addressing conformance issues. > > Example is that we have two parameters in the AFGS1 message. The first is associated with resolution A and apply_grain=1. Second is associated with resolution B and apply_grain=0. > > If the synthesis Is performed at resolution B, then the message says that we shouldn't perform film grain synthesis since apply_grain=0. However, if we strip, the information is lost - and synthesis may use the parameters associated with resolution A instead. Ah, I see, that makes sense. You should put the new flag on the bottom, after bit_depth_chroma, to avoid breaking the ABI. You should also make sure that nothing gets broken. That includes adding apply = 1 in av_film_grain_params_create_side_data() to enable it by default, and making sure that all libavcodec users of the API are updated. Also, better call it simply `apply`, its in a structure called AVFilmGrainParams after all.
Hello. Disclaimer: I'm not the owner/maintainer of any of that code, nor do I know who is. Le ven. 20 sept. 2024 à 02:44, Segall, Andrew via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> a écrit : > case AV_FILM_GRAIN_PARAMS_AV1: > - ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); > + if(fgp->apply_grain) > + ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); > + else > + out->needs_fg = 0; > break; > } Let's imagine AFGS1 would be used for VVC-coded content. It then feels like that logic should be in ff_aom_apply_film_grain, but then apparently, the HEVC frame object and the process has to be at the HEVC decoder level, and not more generic ones. But see below why I'm really commenting this. > @@ -241,6 +241,11 @@ typedef struct AVFilmGrainParams { > */ > enum AVFilmGrainParamsType type; > > > + /** > + * Specifies if film grain parameters are enabled. > + */ > + int apply_grain; > + > /** > * Seed to use for the synthesis process, if the codec allows for it. While it's not abnormal that a generic struct holds things only valid in a few cases, this flag doesn't apply to H.274, and is really processed by AFGS1-aware code, so I'm wondering if that apply_grain should really be in AVFilmGrainAOMParams? Lynne's comment still applies, though.
diff --git a/libavcodec/aom_film_grain.c b/libavcodec/aom_film_grain.c index fdfa3dbf77..251a2793ac 100644 --- a/libavcodec/aom_film_grain.c +++ b/libavcodec/aom_film_grain.c @@ -149,8 +149,10 @@ int ff_aom_parse_film_grain_sets(AVFilmGrainAFGS1Params *s, fgp = &s->sets[set_idx]; aom = &fgp->codec.aom; - fgp->type = get_bits1(gb) ? AV_FILM_GRAIN_PARAMS_AV1 : AV_FILM_GRAIN_PARAMS_NONE; - if (!fgp->type) + fgp->type = AV_FILM_GRAIN_PARAMS_AV1; + + fgp->apply_grain = get_bits1(gb); + if ( !fgp->apply_grain) { payload_bits = get_bits_count(gb) - start_position; if (payload_bits > payload_size * 8) diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c index d915d74d22..8dd4fb10c5 100644 --- a/libavcodec/hevc/hevcdec.c +++ b/libavcodec/hevc/hevcdec.c @@ -3155,7 +3155,10 @@ static int hevc_frame_end(HEVCContext *s) &s->h274db, fgp); break; case AV_FILM_GRAIN_PARAMS_AV1: - ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); + if(fgp->apply_grain) + ret = ff_aom_apply_film_grain(out->frame_grain, out->f, fgp); + else + out->needs_fg = 0; break; } av_assert1(ret >= 0); diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c index f81df9d1bf..08e144ca46 100644 --- a/libavfilter/vf_showinfo.c +++ b/libavfilter/vf_showinfo.c @@ -455,6 +455,7 @@ static void dump_sei_film_grain_params_metadata(AVFilterContext *ctx, const AVFr } av_log(ctx, AV_LOG_INFO, "type %s; ", film_grain_type_names[fgp->type]); + av_log(ctx, AV_LOG_INFO, "apply_grain=%d; ", fgp->apply_grain); av_log(ctx, AV_LOG_INFO, "seed=%"PRIu64"; ", fgp->seed); av_log(ctx, AV_LOG_INFO, "width=%d; ", fgp->width); av_log(ctx, AV_LOG_INFO, "height=%d; ", fgp->height); diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h index ccacab88fe..f3275923e1 100644 --- a/libavutil/film_grain_params.h +++ b/libavutil/film_grain_params.h @@ -241,6 +241,11 @@ typedef struct AVFilmGrainParams { */ enum AVFilmGrainParamsType type; + /** + * Specifies if film grain parameters are enabled. + */ + int apply_grain; + /** * Seed to use for the synthesis process, if the codec allows for it.
Details: Add support for the apply_grain_flag. This allows the film grain process to be enabled/disabled for different display properties. On 9/8/24, 12:06 AM, "Andrew Segall" <asegall@amazon.com <mailto:asegall@amazon.com>> wrote: Signed-off-by: Andrew Segall <asegall@amazon.com <mailto:asegall@amazon.com>> --- libavcodec/aom_film_grain.c | 6 ++++-- libavcodec/hevc/hevcdec.c | 5 ++++- libavfilter/vf_showinfo.c | 1 + libavutil/film_grain_params.h | 5 +++++ 4 files changed, 14 insertions(+), 3 deletions(-) * -- 2.46.0