| Message ID | 20240304130657.30631-7-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/29] lavu/opt: factor per-type dispatch out of av_opt_get() | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 3/4/2024 1:06 PM, Anton Khirnov wrote: > + /** > + * Decoding only. May be set by the caller before avcodec_open2() to an > + * av_malloc()'ed array (or via AVOptions). Owned and freed by the decoder > + * afterwards. > + * > + * By default, when some side data type is present both in global > + * user-supplied coded_side_data and inside the coded bitstream, avcodec > + * will propagate the latter to the decoded frame. > + * > + * This array contains a list of AVPacketSideDataType for which this > + * preference should be switched, i.e. avcodec will prefer global side data > + * over those in stored in the bytestream. It may also contain a single -1, > + * in which case the preference is switched for all side data types. > + */ > + int *side_data_prefer_global; My only comment is that it would be nice to include some text about what the practical application of this option is, rather than just technical details. Something like: This can be useful, when, for example, input contains data in both the container and bitstream. - Derek
Quoting Derek Buitenhuis (2024-03-04 15:05:29) > On 3/4/2024 1:06 PM, Anton Khirnov wrote: > > + /** > > + * Decoding only. May be set by the caller before avcodec_open2() to an > > + * av_malloc()'ed array (or via AVOptions). Owned and freed by the decoder > > + * afterwards. > > + * > > + * By default, when some side data type is present both in global > > + * user-supplied coded_side_data and inside the coded bitstream, avcodec > > + * will propagate the latter to the decoded frame. > > + * > > + * This array contains a list of AVPacketSideDataType for which this > > + * preference should be switched, i.e. avcodec will prefer global side data > > + * over those in stored in the bytestream. It may also contain a single -1, > > + * in which case the preference is switched for all side data types. > > + */ > > + int *side_data_prefer_global; > > My only comment is that it would be nice to include some text about what the practical > application of this option is, rather than just technical details. Something like: > > This can be useful, when, for example, input contains data in both the container > and bitstream. Added text to that effect locally.
On 3/4/2024 10:06 AM, Anton Khirnov wrote: > This and the following commits fix #10857 > --- > doc/APIchanges | 3 +++ > libavcodec/avcodec.h | 20 ++++++++++++++++++++ > libavcodec/decode.c | 36 ++++++++++++++++++++++++++++++++++++ > libavcodec/options_table.h | 13 +++++++++++++ > 4 files changed, 72 insertions(+) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 3209614ed6..5d8585d8ba 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09 > > API changes, most recent first: > > +2024-02-xx - xxxxxxxxxx - lavc 60.xx.100 - avcodec.h > + Add AVCodecContext.[nb_]side_data_prefer_global. > + > 2024-02-xx - xxxxxxxxxx - lavu 58.xx.100 - opt.h > Add AV_OPT_TYPE_FLAG_ARRAY and AVOptionArrayDef. > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 43859251cc..307a3e99db 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -2120,6 +2120,26 @@ typedef struct AVCodecContext { > * an error. > */ > int64_t frame_num; > + > + /** > + * Decoding only. May be set by the caller before avcodec_open2() to an > + * av_malloc()'ed array (or via AVOptions). Owned and freed by the decoder > + * afterwards. > + * > + * By default, when some side data type is present both in global > + * user-supplied coded_side_data and inside the coded bitstream, avcodec > + * will propagate the latter to the decoded frame. > + * > + * This array contains a list of AVPacketSideDataType for which this > + * preference should be switched, i.e. avcodec will prefer global side data > + * over those in stored in the bytestream. It may also contain a single -1, > + * in which case the preference is switched for all side data types. > + */ > + int *side_data_prefer_global; > + /** > + * Number of entries in side_data_prefer_global. > + */ > + unsigned nb_side_data_prefer_global; > } AVCodecContext; > > /** > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 7c67b18bc4..5c80ef9cd0 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -60,6 +60,12 @@ typedef struct DecodeContext { > * The caller has submitted a NULL packet on input. > */ > int draining_started; > + > + /** > + * Bitmask indicating for which side data types we prefer global > + * side data over per-packet. > + */ > + uint64_t side_data_pref_mask; > } DecodeContext; > > static DecodeContext *decode_ctx(AVCodecInternal *avci) > @@ -1744,6 +1750,7 @@ int ff_reget_buffer(AVCodecContext *avctx, AVFrame *frame, int flags) > int ff_decode_preinit(AVCodecContext *avctx) > { > AVCodecInternal *avci = avctx->internal; > + DecodeContext *dc = decode_ctx(avci); > int ret = 0; > > /* if the decoder init function was already called previously, > @@ -1804,6 +1811,35 @@ int ff_decode_preinit(AVCodecContext *avctx) > avctx->export_side_data |= AV_CODEC_EXPORT_DATA_MVS; > } > > + if (avctx->nb_side_data_prefer_global == 1 && > + avctx->side_data_prefer_global[0] == -1) > + dc->side_data_pref_mask = ~0ULL; > + else { > + for (unsigned i = 0; i < avctx->nb_side_data_prefer_global; i++) { > + int val = avctx->side_data_prefer_global[i]; > + > + if (val < 0 || val >= AV_PKT_DATA_NB) { > + av_log(avctx, AV_LOG_ERROR, "Invalid side data type: %d\n", val); > + return AVERROR(EINVAL); > + } > + > + for (unsigned j = 0; j < FF_ARRAY_ELEMS(sd_global_map); j++) { > + if (sd_global_map[j].packet == val) { > + val = sd_global_map[j].frame; > + > + // this code will need to be changed when we have more than > + // 64 frame side data types > + if (val >= 64) { > + av_log(avctx, AV_LOG_ERROR, "Side data type too big\n"); > + return AVERROR_BUG; > + } > + > + dc->side_data_pref_mask |= 1ULL << val; > + } > + } > + } > + } > + > avci->in_pkt = av_packet_alloc(); > avci->last_pkt_props = av_packet_alloc(); > if (!avci->in_pkt || !avci->last_pkt_props) > diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h > index ac32d8928a..3ff03d1cf0 100644 > --- a/libavcodec/options_table.h > +++ b/libavcodec/options_table.h > @@ -42,6 +42,8 @@ > #define D AV_OPT_FLAG_DECODING_PARAM > #define CC AV_OPT_FLAG_CHILD_CONSTS > > +#define AR AV_OPT_TYPE_FLAG_ARRAY > + > #define AV_CODEC_DEFAULT_BITRATE 200*1000 > > static const AVOption avcodec_options[] = { > @@ -405,6 +407,17 @@ static const AVOption avcodec_options[] = { > {"unsafe_output", "allow potentially unsafe hwaccel frame output that might require special care to process successfully", 0, AV_OPT_TYPE_CONST, {.i64 = AV_HWACCEL_FLAG_UNSAFE_OUTPUT }, INT_MIN, INT_MAX, V | D, .unit = "hwaccel_flags"}, > {"extra_hw_frames", "Number of extra hardware frames to allocate for the user", OFFSET(extra_hw_frames), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT_MAX, V|D }, > {"discard_damaged_percentage", "Percentage of damaged samples to discard a frame", OFFSET(discard_damaged_percentage), AV_OPT_TYPE_INT, {.i64 = 95 }, 0, 100, V|D }, > +{"side_data_prefer_global", "Comma-separated list of side data types for which global headers are preferred over frame-level data", > + OFFSET(side_data_prefer_global), AV_OPT_TYPE_INT | AR, .min = -1, .max = INT_MAX, .flags = V|A|S|D, .unit = "side_data_pkt" }, > + {"replaygain", .default_val.i64 = AV_PKT_DATA_REPLAYGAIN, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > + {"displaymatrix", .default_val.i64 = AV_PKT_DATA_DISPLAYMATRIX, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > + {"spherical", .default_val.i64 = AV_PKT_DATA_SPHERICAL, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > + {"stereo3d", .default_val.i64 = AV_PKT_DATA_STEREO3D, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > + {"audio_service_type", .default_val.i64 = AV_PKT_DATA_AUDIO_SERVICE_TYPE, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > + {"mastering_display_metadata", .default_val.i64 = AV_PKT_DATA_MASTERING_DISPLAY_METADATA, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > + {"content_light_level", .default_val.i64 = AV_PKT_DATA_CONTENT_LIGHT_LEVEL, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > + {"icc_profile", .default_val.i64 = AV_PKT_DATA_ICC_PROFILE, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > + {"dynamic_hdr10_plus", .default_val.i64 = AV_PKT_DATA_DYNAMIC_HDR10_PLUS, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, This one packet/frame level only, not global. Is this option meant to also choose which one of those to use? > {NULL}, > }; >
Quoting James Almer (2024-03-05 13:30:58) > > + {"dynamic_hdr10_plus", .default_val.i64 = AV_PKT_DATA_DYNAMIC_HDR10_PLUS, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > > This one packet/frame level only, not global. It is in sd_global_map > Is this option meant to also choose which one of those to use? ???
On 3/5/2024 11:29 AM, Anton Khirnov wrote: > Quoting James Almer (2024-03-05 13:30:58) >>> + {"dynamic_hdr10_plus", .default_val.i64 = AV_PKT_DATA_DYNAMIC_HDR10_PLUS, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, >> >> This one packet/frame level only, not global. > > It is in sd_global_map Then that's a mistake, and I'm probably he culprit. HDR10+ is per frame metadata. Static HDR metadata are mastering_display and ccl. > >> Is this option meant to also choose which one of those to use? > > ??? You can have packet side data at the container level that corresponds to the same thinga per frame side data at the bitstream level does. In HDR10+ case, Matroska may have it in block additional, and then afaik it could be present in the hevc bitstream. One of them should have priority, or the user could be given the choice.
Quoting James Almer (2024-03-05 15:35:02) > On 3/5/2024 11:29 AM, Anton Khirnov wrote: > > Quoting James Almer (2024-03-05 13:30:58) > >>> + {"dynamic_hdr10_plus", .default_val.i64 = AV_PKT_DATA_DYNAMIC_HDR10_PLUS, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, > >> > >> This one packet/frame level only, not global. > > > > It is in sd_global_map > > Then that's a mistake, and I'm probably he culprit. HDR10+ is per frame > metadata. Static HDR metadata are mastering_display and ccl. Ok, dropped from the options table locally. > > > > >> Is this option meant to also choose which one of those to use? > > > > ??? > > You can have packet side data at the container level that corresponds to > the same thinga per frame side data at the bitstream level does. In > HDR10+ case, Matroska may have it in block additional, and then afaik it > could be present in the hevc bitstream. > One of them should have priority, or the user could be given the choice. Right, I've thought about this a bit, but then couldn't find any side data types that some container could export per-packet AND could also be present in the bitstream. One possible solution is to rename the option to something like side_data_prefer_external (better names welcome), and have it switch between user-supplied (i.e. global or per-packet) and in-bitstream side data. This adds an ambiguity for the hypothetical case where some side data exists as global and per-packet - then I'd say lavc should default to per-packet and leave the other case to the caller (should be very rare, possibly could be handled with a bitstream filter).
On 3/5/2024 11:54 AM, Anton Khirnov wrote: > Quoting James Almer (2024-03-05 15:35:02) >> On 3/5/2024 11:29 AM, Anton Khirnov wrote: >>> Quoting James Almer (2024-03-05 13:30:58) >>>>> + {"dynamic_hdr10_plus", .default_val.i64 = AV_PKT_DATA_DYNAMIC_HDR10_PLUS, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, >>>> >>>> This one packet/frame level only, not global. >>> >>> It is in sd_global_map >> >> Then that's a mistake, and I'm probably he culprit. HDR10+ is per frame >> metadata. Static HDR metadata are mastering_display and ccl. > > Ok, dropped from the options table locally. > >> >>> >>>> Is this option meant to also choose which one of those to use? >>> >>> ??? >> >> You can have packet side data at the container level that corresponds to >> the same thinga per frame side data at the bitstream level does. In >> HDR10+ case, Matroska may have it in block additional, and then afaik it >> could be present in the hevc bitstream. >> One of them should have priority, or the user could be given the choice. > > Right, I've thought about this a bit, but then couldn't find any side > data types that some container could export per-packet AND could also be > present in the bitstream. Aside from HDR10+, I'm sure this scenario can also happen with closed captions. > > One possible solution is to rename the option to something like > side_data_prefer_external (better names welcome), and have it switch > between user-supplied (i.e. global or per-packet) and in-bitstream side > data. > > This adds an ambiguity for the hypothetical case where some side data > exists as global and per-packet - then I'd say lavc should default to > per-packet and leave the other case to the caller (should be very rare, > possibly could be handled with a bitstream filter). If a given side data type at the container level were to be duplicated in the header (global) and per packet, then IMO the packet must have priority, given it's the container overriding its own parameters.
Quoting James Almer (2024-03-05 16:07:05) > > > > One possible solution is to rename the option to something like > > side_data_prefer_external (better names welcome), and have it switch > > between user-supplied (i.e. global or per-packet) and in-bitstream side > > data. > > > > This adds an ambiguity for the hypothetical case where some side data > > exists as global and per-packet - then I'd say lavc should default to > > per-packet and leave the other case to the caller (should be very rare, > > possibly could be handled with a bitstream filter). > > If a given side data type at the container level were to be duplicated > in the header (global) and per packet, then IMO the packet must have > priority, given it's the container overriding its own parameters. I'd be careful with 'must' - in a sane world that's what would be reasonable, but I can definitely imagine a situation where you'd want the reverse. But I do agree that it's not something lavc needs to handle.
diff --git a/doc/APIchanges b/doc/APIchanges index 3209614ed6..5d8585d8ba 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09 API changes, most recent first: +2024-02-xx - xxxxxxxxxx - lavc 60.xx.100 - avcodec.h + Add AVCodecContext.[nb_]side_data_prefer_global. + 2024-02-xx - xxxxxxxxxx - lavu 58.xx.100 - opt.h Add AV_OPT_TYPE_FLAG_ARRAY and AVOptionArrayDef. diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 43859251cc..307a3e99db 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -2120,6 +2120,26 @@ typedef struct AVCodecContext { * an error. */ int64_t frame_num; + + /** + * Decoding only. May be set by the caller before avcodec_open2() to an + * av_malloc()'ed array (or via AVOptions). Owned and freed by the decoder + * afterwards. + * + * By default, when some side data type is present both in global + * user-supplied coded_side_data and inside the coded bitstream, avcodec + * will propagate the latter to the decoded frame. + * + * This array contains a list of AVPacketSideDataType for which this + * preference should be switched, i.e. avcodec will prefer global side data + * over those in stored in the bytestream. It may also contain a single -1, + * in which case the preference is switched for all side data types. + */ + int *side_data_prefer_global; + /** + * Number of entries in side_data_prefer_global. + */ + unsigned nb_side_data_prefer_global; } AVCodecContext; /** diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 7c67b18bc4..5c80ef9cd0 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -60,6 +60,12 @@ typedef struct DecodeContext { * The caller has submitted a NULL packet on input. */ int draining_started; + + /** + * Bitmask indicating for which side data types we prefer global + * side data over per-packet. + */ + uint64_t side_data_pref_mask; } DecodeContext; static DecodeContext *decode_ctx(AVCodecInternal *avci) @@ -1744,6 +1750,7 @@ int ff_reget_buffer(AVCodecContext *avctx, AVFrame *frame, int flags) int ff_decode_preinit(AVCodecContext *avctx) { AVCodecInternal *avci = avctx->internal; + DecodeContext *dc = decode_ctx(avci); int ret = 0; /* if the decoder init function was already called previously, @@ -1804,6 +1811,35 @@ int ff_decode_preinit(AVCodecContext *avctx) avctx->export_side_data |= AV_CODEC_EXPORT_DATA_MVS; } + if (avctx->nb_side_data_prefer_global == 1 && + avctx->side_data_prefer_global[0] == -1) + dc->side_data_pref_mask = ~0ULL; + else { + for (unsigned i = 0; i < avctx->nb_side_data_prefer_global; i++) { + int val = avctx->side_data_prefer_global[i]; + + if (val < 0 || val >= AV_PKT_DATA_NB) { + av_log(avctx, AV_LOG_ERROR, "Invalid side data type: %d\n", val); + return AVERROR(EINVAL); + } + + for (unsigned j = 0; j < FF_ARRAY_ELEMS(sd_global_map); j++) { + if (sd_global_map[j].packet == val) { + val = sd_global_map[j].frame; + + // this code will need to be changed when we have more than + // 64 frame side data types + if (val >= 64) { + av_log(avctx, AV_LOG_ERROR, "Side data type too big\n"); + return AVERROR_BUG; + } + + dc->side_data_pref_mask |= 1ULL << val; + } + } + } + } + avci->in_pkt = av_packet_alloc(); avci->last_pkt_props = av_packet_alloc(); if (!avci->in_pkt || !avci->last_pkt_props) diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h index ac32d8928a..3ff03d1cf0 100644 --- a/libavcodec/options_table.h +++ b/libavcodec/options_table.h @@ -42,6 +42,8 @@ #define D AV_OPT_FLAG_DECODING_PARAM #define CC AV_OPT_FLAG_CHILD_CONSTS +#define AR AV_OPT_TYPE_FLAG_ARRAY + #define AV_CODEC_DEFAULT_BITRATE 200*1000 static const AVOption avcodec_options[] = { @@ -405,6 +407,17 @@ static const AVOption avcodec_options[] = { {"unsafe_output", "allow potentially unsafe hwaccel frame output that might require special care to process successfully", 0, AV_OPT_TYPE_CONST, {.i64 = AV_HWACCEL_FLAG_UNSAFE_OUTPUT }, INT_MIN, INT_MAX, V | D, .unit = "hwaccel_flags"}, {"extra_hw_frames", "Number of extra hardware frames to allocate for the user", OFFSET(extra_hw_frames), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT_MAX, V|D }, {"discard_damaged_percentage", "Percentage of damaged samples to discard a frame", OFFSET(discard_damaged_percentage), AV_OPT_TYPE_INT, {.i64 = 95 }, 0, 100, V|D }, +{"side_data_prefer_global", "Comma-separated list of side data types for which global headers are preferred over frame-level data", + OFFSET(side_data_prefer_global), AV_OPT_TYPE_INT | AR, .min = -1, .max = INT_MAX, .flags = V|A|S|D, .unit = "side_data_pkt" }, + {"replaygain", .default_val.i64 = AV_PKT_DATA_REPLAYGAIN, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, + {"displaymatrix", .default_val.i64 = AV_PKT_DATA_DISPLAYMATRIX, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, + {"spherical", .default_val.i64 = AV_PKT_DATA_SPHERICAL, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, + {"stereo3d", .default_val.i64 = AV_PKT_DATA_STEREO3D, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, + {"audio_service_type", .default_val.i64 = AV_PKT_DATA_AUDIO_SERVICE_TYPE, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, + {"mastering_display_metadata", .default_val.i64 = AV_PKT_DATA_MASTERING_DISPLAY_METADATA, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, + {"content_light_level", .default_val.i64 = AV_PKT_DATA_CONTENT_LIGHT_LEVEL, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, + {"icc_profile", .default_val.i64 = AV_PKT_DATA_ICC_PROFILE, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, + {"dynamic_hdr10_plus", .default_val.i64 = AV_PKT_DATA_DYNAMIC_HDR10_PLUS, .type = AV_OPT_TYPE_CONST, .flags = A|D, .unit = "side_data_pkt" }, {NULL}, };