diff mbox series

[FFmpeg-devel,2/3] - libavcodec/aom_film_grain: Add apply_grain flag

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

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/configure_x86 warning Failed to apply patch

Commit Message

Segall, Andrew via ffmpeg-devel Sept. 20, 2024, 12:44 a.m. UTC 
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

Comments

Lynne Sept. 20, 2024, 7:14 a.m. UTC | #1 
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?
Segall, Andrew via ffmpeg-devel Sept. 20, 2024, 7:42 a.m. UTC | #2 
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.
Lynne Sept. 20, 2024, 10:06 a.m. UTC | #3 
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.
Christophe Gisquet Sept. 20, 2024, 1:09 p.m. UTC | #4 
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 mbox series

Patch

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.