[FFmpeg-devel] [PATCH v10 00/14] encoder AVCodecContext configuration side data

[James Almer]

On 3/18/2024 6:31 PM, Jan Ekström wrote:
> Differences to v9:
> 1. rebased on top of current master
> 2. renamed the avctx AVFrameSideData array according to Anton's naming sense,
>     as at this point that is now OK by James as well and I just want to get this
>     done with.
> 3. removed the avctx helper to get through review.
> 4. added "Afterwards owned and freed by the encoder" comment into the
>     description of the side data array.
> 5. added APIchanges (missing the AVBufferRef function since that is for now to
>     be skipped) and avutil/avcodec bumps.
> 6. removed all references to "_set" from internal functions and the newly added
>     test. The only thing that is left around that is a set is a 100% internal
>     helper struct for the side_data_array test called FrameSideDataSet.
> 
> Comparison URL (mostly configure and wrappers, avutil/frame.c):
> https://github.com/jeeb/ffmpeg/compare/avcodec_cll_mdcv_side_data_v9..avcodec_cll_mdcv_side_data_v10
> 
> This patch set I've now been working for a while since I felt like it was weird
> we couldn't pass through information such as static HDR metadata to encoders
> from decoded input. This initial version adds the necessary framework, as well
> as adds static HDR metadata support for libsvtav1, libx264 as well as libx265
> wrappers.
> 
> An alternative to this would be to make encoders only properly initialize when
> they receive the first AVFrame, but that seems to be a bigger, nastier change
> than introducing an AVFrameSideDataSet in avctx as everything seems to
> presume that extradata etc are available after opening the encoder.
> 
> Note: Any opinions on whether FFCodec or AVCodec should have
>        handled_side_data list, so that if format specific generic logic is
>        added, it could be checked whether the codec itself handles this side
>        data? This could also be utilized to list handled side data from f.ex.
>        `ffmpeg -h encoder=libsvtav1`.
> 
> Jan
> 
> Jan Ekström (14):
>    avutil/frame: split side data list wiping out to non-AVFrame function
>    avutil/frame: add helper for freeing arrays of side data
>    avutil/frame: split side_data_from_buf to base and AVFrame func
>    avutil/frame: split side data removal out to non-AVFrame function
>    avutil/frame: add helper for adding side data to array
>    avutil/frame: add helper for adding existing side data to array
>    avutil/frame: add helper for adding side data w/ AVBufferRef to array
>    avutil/frame: add helper for getting side data from array
>    {avutil/version,APIchanges}: bump, document new AVFrameSideData
>      functions
>    avcodec: add frame side data array to AVCodecContext
>    ffmpeg: pass first video AVFrame's side data to encoder
>    avcodec/libsvtav1: add support for writing out CLL and MDCV
>    avcodec/libx264: add support for writing out CLL and MDCV
>    avcodec/libx265: add support for writing out CLL and MDCV
> 
>   configure                         |   2 +
>   doc/APIchanges                    |   8 ++
>   fftools/ffmpeg_enc.c              |  15 +++
>   libavcodec/avcodec.h              |  13 +++
>   libavcodec/libsvtav1.c            |  69 ++++++++++++
>   libavcodec/libx264.c              |  80 +++++++++++++
>   libavcodec/libx265.c              |  89 +++++++++++++++
>   libavcodec/options.c              |   2 +
>   libavcodec/version.h              |   4 +-
>   libavutil/Makefile                |   1 +
>   libavutil/frame.c                 | 180 +++++++++++++++++++++++++-----
>   libavutil/frame.h                 |  88 +++++++++++++++
>   libavutil/tests/side_data_array.c | 103 +++++++++++++++++
>   libavutil/version.h               |   2 +-
>   tests/fate/enc_external.mak       |  15 +++
>   tests/fate/libavutil.mak          |   4 +
>   tests/ref/fate/libsvtav1-hdr10    |  14 +++
>   tests/ref/fate/libx264-hdr10      |  15 +++
>   tests/ref/fate/libx265-hdr10      |  16 +++
>   tests/ref/fate/side_data_array    |  14 +++
>   20 files changed, 701 insertions(+), 33 deletions(-)
>   create mode 100644 libavutil/tests/side_data_array.c
>   create mode 100644 tests/ref/fate/libsvtav1-hdr10
>   create mode 100644 tests/ref/fate/libx264-hdr10
>   create mode 100644 tests/ref/fate/libx265-hdr10
>   create mode 100644 tests/ref/fate/side_data_array

LGTM. Except for patch 7 which is still in discussion and is currently 
not required, the rest can go in.