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

[Lynne]

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 at ffmpeg.org <mailto:ffmpeg-devel-bounces at ffmpeg.org> on behalf of ffmpeg-devel at ffmpeg.org <mailto:ffmpeg-devel at 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 at amazon.com <mailto:asegall at amazon.com> <mailto:asegall at amazon.com <mailto:asegall at amazon.com>>> wrote:
>>
>>
>> Signed-off-by: Andrew Segall <asegall at amazon.com <mailto:asegall at amazon.com> <mailto:asegall at amazon.com <mailto:asegall at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xA2FEA5F03F034464.asc
Type: application/pgp-keys
Size: 624 bytes
Desc: OpenPGP public key
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240920/2ee15d16/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240920/2ee15d16/attachment.sig>