Message ID | 20240318213141.1376789-1-jeebjp@gmail.com |
---|---|
State | New |
Headers | show |
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.
On Wed, Mar 20, 2024 at 7:00 PM James Almer <jamrial@gmail.com> wrote: > > 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. Thanks. Applied everything except for "avutil/frame: add helper for adding side data w/ AVBufferRef to array". Jan
diff --git a/doc/APIchanges b/doc/APIchanges index a44c8e4f10..a025f1df14 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -2,6 +2,14 @@ The last version increases of all libraries were on 2024-03-07 API changes, most recent first: +2024-03-xx - xxxxxxxxxx - lavc 61.2.100 - avcodec.h + Add AVCodecContext.[nb_]decoded_side_data. + +2024-03-xx - xxxxxxxxxx - lavu 59.3.100 - frame.h + Add av_frame_side_data_free(), av_frame_side_data_new(), + av_frame_side_data_clone(), av_frame_side_data_get() as well + as AV_FRAME_SIDE_DATA_FLAG_UNIQUE. + 2024-03-xx - xxxxxxxxxx - lavu 59.2.100 - channel_layout.h Add AV_CHANNEL_LAYOUT_RETYPE_FLAG_CANONICAL. diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c index c8d15fb999..f01be1c22f 100644 --- a/fftools/ffmpeg_enc.c +++ b/fftools/ffmpeg_enc.c @@ -246,14 +246,19 @@ int enc_open(void *opaque, const AVFrame *frame) enc_ctx->colorspace = frame->colorspace; enc_ctx->chroma_sample_location = frame->chroma_location; - ret = avcodec_configure_side_data( - enc_ctx, - (const AVFrameSideData **)frame->side_data, frame->nb_side_data, - AV_FRAME_SIDE_DATA_FLAG_UNIQUE); - if (ret < 0) { - av_log(NULL, AV_LOG_ERROR, "failed to configure video encoder: %s!\n", - av_err2str(ret)); - return ret; + for (int i = 0; i < frame->nb_side_data; i++) { + ret = av_frame_side_data_clone( + &enc_ctx->decoded_side_data, &enc_ctx->nb_decoded_side_data, + frame->side_data[i], AV_FRAME_SIDE_DATA_FLAG_UNIQUE); + if (ret < 0) { + av_frame_side_data_free( + &enc_ctx->decoded_side_data, + &enc_ctx->nb_decoded_side_data); + av_log(NULL, AV_LOG_ERROR, + "failed to configure video encoder: %s!\n", + av_err2str(ret)); + return ret; + } } if (enc_ctx->flags & (AV_CODEC_FLAG_INTERLACED_DCT | AV_CODEC_FLAG_INTERLACED_ME) || diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index 3183952172..a9a87bb58c 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -686,34 +686,3 @@ int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *fr return ff_decode_receive_frame(avctx, frame); return ff_encode_receive_frame(avctx, frame); } - -int avcodec_configure_side_data(AVCodecContext *avctx, - const AVFrameSideData **sd, const int nb_sd, - unsigned int flags) -{ - if (!avctx) - return AVERROR(EINVAL); - - if (!sd) { - av_frame_side_data_free( - &avctx->encoder_side_data, &avctx->nb_encoder_side_data); - return 0; - } - - if (nb_sd > 0 && nb_sd == avctx->nb_encoder_side_data && - sd == (const AVFrameSideData **)avctx->encoder_side_data) - return AVERROR(EINVAL); - - for (int i = 0; i < nb_sd; i++) { - int ret = av_frame_side_data_clone( - &avctx->encoder_side_data, &avctx->nb_encoder_side_data, sd[i], - flags); - if (ret < 0) { - av_frame_side_data_free( - &avctx->encoder_side_data, &avctx->nb_encoder_side_data); - return ret; - } - } - - return 0; -} diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 01085cc3c8..83dc487251 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -2069,11 +2069,12 @@ typedef struct AVCodecContext { * libavutil/frame.h. * * - encoding: may be set by user before calling avcodec_open2() for - * encoder configuration. + * encoder configuration. Afterwards owned and freed by the + * encoder. * - decoding: unused */ - AVFrameSideData **encoder_side_data; - int nb_encoder_side_data; + AVFrameSideData **decoded_side_data; + int nb_decoded_side_data; } AVCodecContext; /** @@ -3079,27 +3080,6 @@ void av_fast_padded_mallocz(void *ptr, unsigned int *size, size_t min_size); */ int avcodec_is_open(AVCodecContext *s); -/** - * Add multiple side data entries to an AVCodecContext's array in one go, for - * example from an AVFrame. - * - * In case the function fails to add a side data entry, it will clear the - * whole side data set. - * - * @param avctx context to which the side data should be added - * @param sd array of side data to use as input. - * if null, clears out the side data for this context. - * @param nb_sd integer containing the number of entries in the array. - * @param flags Some combination of AV_FRAME_SIDE_DATA_SET_FLAG_* flags, or 0. - * - * @return negative error code on failure, >=0 on success. - * - * @see av_frame_side_data_new regarding the flags. - */ -int avcodec_configure_side_data(AVCodecContext *avctx, - const AVFrameSideData **sd, const int nb_sd, - unsigned int flags); - /** * @} */ diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c index 73bfdba3cf..6400a6507a 100644 --- a/libavcodec/libsvtav1.c +++ b/libavcodec/libsvtav1.c @@ -181,12 +181,12 @@ static void handle_side_data(AVCodecContext *avctx, { const AVFrameSideData *cll_sd = av_frame_side_data_get( - (const AVFrameSideData **)avctx->encoder_side_data, - avctx->nb_encoder_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); + (const AVFrameSideData **)avctx->decoded_side_data, + avctx->nb_decoded_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); const AVFrameSideData *mdcv_sd = av_frame_side_data_get( - (const AVFrameSideData **)avctx->encoder_side_data, - avctx->nb_encoder_side_data, + (const AVFrameSideData **)avctx->decoded_side_data, + avctx->nb_decoded_side_data, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA); if (cll_sd) { diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index 2a8d71cd0b..4804dae6e9 100644 --- a/libavcodec/libx264.c +++ b/libavcodec/libx264.c @@ -907,12 +907,12 @@ static void handle_side_data(AVCodecContext *avctx, x264_param_t *params) #if CONFIG_LIBX264_HDR10 const AVFrameSideData *cll_sd = av_frame_side_data_get( - (const AVFrameSideData **)avctx->encoder_side_data, - avctx->nb_encoder_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); + (const AVFrameSideData **)avctx->decoded_side_data, + avctx->nb_decoded_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); const AVFrameSideData *mdcv_sd = av_frame_side_data_get( - (const AVFrameSideData **)avctx->encoder_side_data, - avctx->nb_encoder_side_data, + (const AVFrameSideData **)avctx->decoded_side_data, + avctx->nb_decoded_side_data, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA); if (cll_sd) { diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c index 0c884fe04d..70ec6d3539 100644 --- a/libavcodec/libx265.c +++ b/libavcodec/libx265.c @@ -231,12 +231,12 @@ static int handle_side_data(AVCodecContext *avctx, const x265_api *api, { const AVFrameSideData *cll_sd = av_frame_side_data_get( - (const AVFrameSideData **)avctx->encoder_side_data, - avctx->nb_encoder_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); + (const AVFrameSideData **)avctx->decoded_side_data, + avctx->nb_decoded_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL); const AVFrameSideData *mdcv_sd = av_frame_side_data_get( - (const AVFrameSideData **)avctx->encoder_side_data, - avctx->nb_encoder_side_data, + (const AVFrameSideData **)avctx->decoded_side_data, + avctx->nb_decoded_side_data, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA); if (cll_sd) { diff --git a/libavcodec/options.c b/libavcodec/options.c index dd5c697da3..5169f2e476 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -177,7 +177,7 @@ void avcodec_free_context(AVCodecContext **pavctx) av_freep(&avctx->rc_override); av_channel_layout_uninit(&avctx->ch_layout); av_frame_side_data_free( - &avctx->encoder_side_data, &avctx->nb_encoder_side_data); + &avctx->decoded_side_data, &avctx->nb_decoded_side_data); av_freep(pavctx); } diff --git a/libavcodec/version.h b/libavcodec/version.h index b4616ccc27..0550d7b0d8 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -29,8 +29,8 @@ #include "version_major.h" -#define LIBAVCODEC_VERSION_MINOR 1 -#define LIBAVCODEC_VERSION_MICRO 101 +#define LIBAVCODEC_VERSION_MINOR 2 +#define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ LIBAVCODEC_VERSION_MINOR, \ diff --git a/libavutil/Makefile b/libavutil/Makefile index 4415c913a1..008fcfcd9c 100644 --- a/libavutil/Makefile +++ b/libavutil/Makefile @@ -271,7 +271,7 @@ TESTPROGS = adler32 \ ripemd \ sha \ sha512 \ - side_data_set \ + side_data_array \ softfloat \ tree \ twofish \ diff --git a/libavutil/frame.c b/libavutil/frame.c index 47ecd964b8..e8dfd4d926 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -711,10 +711,10 @@ AVBufferRef *av_frame_get_plane_buffer(const AVFrame *frame, int plane) return NULL; } -static AVFrameSideData *add_side_data_to_set_from_buf(AVFrameSideData ***sd, - int *nb_sd, - enum AVFrameSideDataType type, - AVBufferRef *buf) +static AVFrameSideData *add_side_data_from_buf(AVFrameSideData ***sd, + int *nb_sd, + enum AVFrameSideDataType type, + AVBufferRef *buf) { AVFrameSideData *ret, **tmp; @@ -748,8 +748,8 @@ AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame, AVBufferRef *buf) { return - add_side_data_to_set_from_buf(&frame->side_data, &frame->nb_side_data, - type, buf); + add_side_data_from_buf( + &frame->side_data, &frame->nb_side_data, type, buf); } AVFrameSideData *av_frame_new_side_data(AVFrame *frame, @@ -774,7 +774,7 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd, if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE) remove_side_data(sd, nb_sd, type); - ret = add_side_data_to_set_from_buf(sd, nb_sd, type, buf); + ret = add_side_data_from_buf(sd, nb_sd, type, buf); if (!ret) av_buffer_unref(&buf); @@ -799,7 +799,7 @@ AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd, if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE) remove_side_data(sd, nb_sd, type); - sd_dst = add_side_data_to_set_from_buf(sd, nb_sd, type, new_buf); + sd_dst = add_side_data_from_buf(sd, nb_sd, type, new_buf); if (!sd_dst) { av_buffer_unref(&new_buf); return NULL; diff --git a/libavutil/tests/side_data_set.c b/libavutil/tests/side_data_array.c similarity index 100% rename from libavutil/tests/side_data_set.c rename to libavutil/tests/side_data_array.c diff --git a/libavutil/version.h b/libavutil/version.h index 57cad02ec0..5027b025be 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 59 -#define LIBAVUTIL_VERSION_MINOR 2 +#define LIBAVUTIL_VERSION_MINOR 3 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ diff --git a/tests/fate/libavutil.mak b/tests/fate/libavutil.mak index 6864ea9c03..6bf03b2438 100644 --- a/tests/fate/libavutil.mak +++ b/tests/fate/libavutil.mak @@ -148,9 +148,9 @@ FATE_LIBAVUTIL += fate-sha512 fate-sha512: libavutil/tests/sha512$(EXESUF) fate-sha512: CMD = run libavutil/tests/sha512$(EXESUF) -FATE_LIBAVUTIL += fate-side_data_set -fate-side_data_set: libavutil/tests/side_data_set$(EXESUF) -fate-side_data_set: CMD = run libavutil/tests/side_data_set$(EXESUF) +FATE_LIBAVUTIL += fate-side_data_array +fate-side_data_array: libavutil/tests/side_data_array$(EXESUF) +fate-side_data_array: CMD = run libavutil/tests/side_data_array$(EXESUF) FATE_LIBAVUTIL += fate-tree fate-tree: libavutil/tests/tree$(EXESUF) diff --git a/tests/ref/fate/side_data_set b/tests/ref/fate/side_data_array similarity index 100% rename from tests/ref/fate/side_data_set rename to tests/ref/fate/side_data_array