[FFmpeg-devel] [PATCH v2 1/3] libavutil: introduce AVFilmGrainParams side data
James Almer
jamrial at gmail.com
Sun Nov 22 22:28:59 EET 2020
On 11/12/2020 4:28 PM, Lynne wrote:
> Nov 12, 2020, 19:46 by jamrial at gmail.com:
>
>> On 11/12/2020 2:42 PM, Lynne wrote:
>>
>>> This patch introduces a new frame side data type AVFilmGrainParams for use
>>> with video codecs which are able to use it.
>>>
>>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>>> the same data, as well as the fact there already exist rarely-used specifications
>>> for both H.264 and HEVC.
>>>
>>> Patch attached.
>>>
>>> From 522e3a80f44129c98f0c564d44815dbe6a740568 Mon Sep 17 00:00:00 2001
>>> From: Lynne <dev at lynne.ee>
>>> Date: Thu, 12 Nov 2020 12:44:30 +0100
>>> Subject: [PATCH v2 1/3] libavutil: introduce AVFilmGrainParams side data
>>>
>>> This patch introduces a new frame side data type AVFilmGrainParams for use
>>> with video codecs which are able to use it.
>>>
>>> It is generalized rather than being AV1 specific as AV2 is expected to carry
>>> the same data, as well as the fact there already exist rarely-used specifications
>>> for both H.264 and HEVC.
>>> ---
>>> doc/APIchanges | 4 +
>>> libavutil/Makefile | 2 +
>>> libavutil/film_grain_params.c | 42 +++++++++
>>> libavutil/film_grain_params.h | 159 ++++++++++++++++++++++++++++++++++
>>> libavutil/frame.c | 1 +
>>> libavutil/frame.h | 6 ++
>>> libavutil/version.h | 2 +-
>>> 7 files changed, 215 insertions(+), 1 deletion(-)
>>> create mode 100644 libavutil/film_grain_params.c
>>> create mode 100644 libavutil/film_grain_params.h
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index b70c78a483..41248724d9 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -15,6 +15,10 @@ libavutil: 2017-10-21
>>> API changes, most recent first:
>>> +2020-xx-xx - xxxxxxxxxx - lavu 56.61.100 - film_grain_params.h
>>> + Adds a new API for extracting codec film grain parameters as side data.
>>> + Adds a new AVFrameSideDataType entry AV_FRAME_DATA_FILM_GRAIN_PARAMS for it.
>>> +
>>> 2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h
>>> Add AVSTREAM_EVENT_FLAG_NEW_PACKETS.
>>> diff --git a/libavutil/Makefile b/libavutil/Makefile
>>> index 9b08372eb2..27bafe9e12 100644
>>> --- a/libavutil/Makefile
>>> +++ b/libavutil/Makefile
>>> @@ -84,6 +84,7 @@ HEADERS = adler32.h \
>>> xtea.h \
>>> tea.h \
>>> tx.h \
>>> + film_grain_params.h \
>>> HEADERS-$(CONFIG_LZO) += lzo.h
>>> @@ -170,6 +171,7 @@ OBJS = adler32.o \
>>> tx_double.o \
>>> tx_int32.o \
>>> video_enc_params.o \
>>> + film_grain_params.o \
>>> OBJS-$(CONFIG_CUDA) += hwcontext_cuda.o
>>> diff --git a/libavutil/film_grain_params.c b/libavutil/film_grain_params.c
>>> new file mode 100644
>>> index 0000000000..930d23c7fe
>>> --- /dev/null
>>> +++ b/libavutil/film_grain_params.c
>>> @@ -0,0 +1,42 @@
>>> +/**
>>> + * This file is part of FFmpeg.
>>> + *
>>> + * FFmpeg is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * FFmpeg is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with FFmpeg; if not, write to the Free Software
>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>> + */
>>> +
>>> +#include "film_grain_params.h"
>>> +
>>> +AVFilmGrainParams *av_film_grain_params_alloc(size_t *size)
>>> +{
>>> + AVFilmGrainParams *params = av_mallocz(sizeof(AVFilmGrainParams));
>>> +
>>> + if (size)
>>> + *size = sizeof(*params);
>>> +
>>> + return params;
>>> +}
>>> +
>>> +AVFilmGrainParams *av_film_grain_params_create_side_data(AVFrame *frame)
>>> +{
>>> + AVFrameSideData *side_data = av_frame_new_side_data(frame,
>>> + AV_FRAME_DATA_FILM_GRAIN_PARAMS,
>>> + sizeof(AVFilmGrainParams));
>>> + if (!side_data)
>>> + return NULL;
>>> +
>>> + memset(side_data->data, 0, sizeof(AVFilmGrainParams));
>>> +
>>> + return (AVFilmGrainParams *)side_data->data;
>>> +}
>>> diff --git a/libavutil/film_grain_params.h b/libavutil/film_grain_params.h
>>> new file mode 100644
>>> index 0000000000..ba20fe7fa6
>>> --- /dev/null
>>> +++ b/libavutil/film_grain_params.h
>>> @@ -0,0 +1,159 @@
>>> +/*
>>> + * Copyright (c) 2016 Neil Birkbeck <neil.birkbeck at gmail.com>
>>>
>>
>> Copy paste mistake?
>>
>
> Yup.
>
>
>
>>> +typedef struct AVFilmGrainAOMParams {
>>>
>>
>> If you believe AV2 will have a compatible implementation, then this name is fine.
>>
>
> I don't think they're going to change it at all. They haven't even proven all this
> system is capable of in order to replace it with something better, so they might
> just do some minor tweaks. After all, its already somewhat ossified thanks to
> hardware.
>
>
>
>>> + int8_t ar_coeffs_uv[2][25 + 3 /* padding for alignment purposes */];
>>>
>> Shouldn't the compiler take care of alignment?
>>
>
> This simplified dav1d's SIMD, hence why its there, so I decided to leave it as-is.
> The comment above says so too, but this small comment draws too much attention,
> so I removed it.
I don't think that's something we should consider in our public API. We
should prioritize small portable structs, and not trying to optimize
fields for one potential use in one potential target out of many.
This applies to every other case where you gave SIMD as reason.
>
>
>
>>> +
>>> + /**
>>> + * Specifies the range of the auto-regressive coefficients. Values of 6,
>>> + * 7, 8 and so on represent a range of [-2, 2), [-1, 1), [-0.5, 0.5) and
>>> + * so on. For AV1 must be between 6 and 9.
>>> + */
>>> + uint64_t ar_coeff_shift;
>>>
>>
>> Why uint64_t? Sounds like it may be excessive for this.
>>
>
> Same as above, simplifies SIMD. Directly used as a psrad memory arg.
>
>
>
>>> +
>>> + /**
>>> + * Signals the down shift applied to the generated gaussian numbers during
>>> + * synthesis.
>>> + */
>>> + int grain_scale_shift;
>>> +
>>> + /**
>>> + * Specifies the luma/chroma multipliers for the index to the component
>>> + * scaling function.
>>> + */
>>> + int uv_mult[2];
>>> + int uv_mult_luma[2];
>>>
>>
>> int8_t, like ar_coeffs_y. Or could AV2 increase the range considerably?
>>
>
> Same as above, SIMD.
>
>
>
>>> +
>>> + /**
>>> + * Offset used for component scaling function. For AV1 its a 9-bit value
>>> + * with a range [-256, 255] */
>>> + int uv_offset[2];
>>>
>>
>> int16_t.
>>
>
> Could be changed, but there's a single movd in dav1d's SSSE3 SIMD.
> Not sure if it worth it.
>
>
>
>>> +
>>> + /**
>>> + * Signals whether to overlap film grain blocks.
>>> + */
>>> + int overlap_flag;
>>> +} AVFilmGrainAOMParams;
>>> +
>>> +typedef struct AVFilmGrainParams {
>>> + /**
>>> + * Specifies the codec for which this structure is valid.
>>> + */
>>> + enum AVFilmGrainParamsType type;
>>> +
>>> + /**
>>> + * Seed to use for the synthesis process, if the codec allows for it.
>>> + */
>>> + uint32_t seed;
>>>
>>
>> Maybe uint64_t, just in case. This is meant to apply to all implementations.
>>
>
> Done.
>
> Both changes applied locally.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list