| Message ID | 20230927131242.1950-10-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | AVCodecContext and AVCodecParameters side data | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting James Almer (2023-09-27 15:12:40) > avcodec/decode: check for global side data in AVCodecContext side data I don't think this makes it clear what this commit actually does. Make it something like 'propagate global side data to frames'. > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/decode.c | 48 +++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 40 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index a7196b5740..3b4bb70689 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -1422,6 +1422,20 @@ static int add_metadata_from_side_data(const AVPacket *avpkt, AVFrame *frame) > return av_packet_unpack_dictionary(side_metadata, size, frame_md); > } > > +static const struct { > + enum AVPacketSideDataType packet; > + enum AVFrameSideDataType frame; > +} sd_global_map[] = { > + { AV_PKT_DATA_REPLAYGAIN , AV_FRAME_DATA_REPLAYGAIN }, > + { AV_PKT_DATA_SPHERICAL, AV_FRAME_DATA_SPHERICAL }, > + { AV_PKT_DATA_STEREO3D, AV_FRAME_DATA_STEREO3D }, > + { AV_PKT_DATA_AUDIO_SERVICE_TYPE, AV_FRAME_DATA_AUDIO_SERVICE_TYPE }, > + { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA }, > + { AV_PKT_DATA_CONTENT_LIGHT_LEVEL, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL }, > + { AV_PKT_DATA_ICC_PROFILE, AV_FRAME_DATA_ICC_PROFILE }, > + { AV_PKT_DATA_DYNAMIC_HDR10_PLUS, AV_FRAME_DATA_DYNAMIC_HDR_PLUS }, > +}; > + > int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx, > AVFrame *frame, const AVPacket *pkt) > { > @@ -1429,18 +1443,10 @@ int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx, > enum AVPacketSideDataType packet; > enum AVFrameSideDataType frame; > } sd[] = { > - { AV_PKT_DATA_REPLAYGAIN , AV_FRAME_DATA_REPLAYGAIN }, > { AV_PKT_DATA_DISPLAYMATRIX, AV_FRAME_DATA_DISPLAYMATRIX }, Why are you leaving displaymatrix out? Also, what happens if the same side data is present at both global and packet level? Won't you get two instances in the frame? I think the correct behaviour would be that packet overrides global.
On 10/3/2023 9:34 AM, Anton Khirnov wrote: > Quoting James Almer (2023-09-27 15:12:40) >> avcodec/decode: check for global side data in AVCodecContext side data > > I don't think this makes it clear what this commit actually does. > Make it something like 'propagate global side data to frames'. > >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/decode.c | 48 +++++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 40 insertions(+), 8 deletions(-) >> >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >> index a7196b5740..3b4bb70689 100644 >> --- a/libavcodec/decode.c >> +++ b/libavcodec/decode.c >> @@ -1422,6 +1422,20 @@ static int add_metadata_from_side_data(const AVPacket *avpkt, AVFrame *frame) >> return av_packet_unpack_dictionary(side_metadata, size, frame_md); >> } >> >> +static const struct { >> + enum AVPacketSideDataType packet; >> + enum AVFrameSideDataType frame; >> +} sd_global_map[] = { >> + { AV_PKT_DATA_REPLAYGAIN , AV_FRAME_DATA_REPLAYGAIN }, >> + { AV_PKT_DATA_SPHERICAL, AV_FRAME_DATA_SPHERICAL }, >> + { AV_PKT_DATA_STEREO3D, AV_FRAME_DATA_STEREO3D }, >> + { AV_PKT_DATA_AUDIO_SERVICE_TYPE, AV_FRAME_DATA_AUDIO_SERVICE_TYPE }, >> + { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA }, >> + { AV_PKT_DATA_CONTENT_LIGHT_LEVEL, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL }, >> + { AV_PKT_DATA_ICC_PROFILE, AV_FRAME_DATA_ICC_PROFILE }, >> + { AV_PKT_DATA_DYNAMIC_HDR10_PLUS, AV_FRAME_DATA_DYNAMIC_HDR_PLUS }, >> +}; >> + >> int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx, >> AVFrame *frame, const AVPacket *pkt) >> { >> @@ -1429,18 +1443,10 @@ int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx, >> enum AVPacketSideDataType packet; >> enum AVFrameSideDataType frame; >> } sd[] = { >> - { AV_PKT_DATA_REPLAYGAIN , AV_FRAME_DATA_REPLAYGAIN }, >> { AV_PKT_DATA_DISPLAYMATRIX, AV_FRAME_DATA_DISPLAYMATRIX }, > > Why are you leaving displaymatrix out? See the code patch 10/11 removes. Display matrix in global side data is apparently not meant to be propagated to frames. If i move this line to the global map, a couple tests change their output due to new side data in frames. If you want that to happen (After all, the CLI is not the only library user), I'd rather remove the skipping code in the CLI first, with the relevant test refs changes, so this patch is not the one to change them. > > Also, what happens if the same side data is present at both global and > packet level? Won't you get two instances in the frame? > I think the correct behaviour would be that packet overrides global. I'm keeping the behavior from before this set intact with this at Andreas' request. In a previous iteration i ensured packet side data would replace global side data in the output frames, but Andreas asked me to not do that, as a decoder could add that same side data type and it should have priority over container level side data (All entries will be added to the frame anyway).
On 10/3/2023 3:46 PM, James Almer wrote: > On 10/3/2023 9:34 AM, Anton Khirnov wrote: >> Quoting James Almer (2023-09-27 15:12:40) >>> avcodec/decode: check for global side data in AVCodecContext side data >> >> I don't think this makes it clear what this commit actually does. >> Make it something like 'propagate global side data to frames'. >> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/decode.c | 48 +++++++++++++++++++++++++++++++++++++-------- >>> 1 file changed, 40 insertions(+), 8 deletions(-) >>> >>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>> index a7196b5740..3b4bb70689 100644 >>> --- a/libavcodec/decode.c >>> +++ b/libavcodec/decode.c >>> @@ -1422,6 +1422,20 @@ static int add_metadata_from_side_data(const >>> AVPacket *avpkt, AVFrame *frame) >>> return av_packet_unpack_dictionary(side_metadata, size, frame_md); >>> } >>> +static const struct { >>> + enum AVPacketSideDataType packet; >>> + enum AVFrameSideDataType frame; >>> +} sd_global_map[] = { >>> + { AV_PKT_DATA_REPLAYGAIN , >>> AV_FRAME_DATA_REPLAYGAIN }, >>> + { AV_PKT_DATA_SPHERICAL, >>> AV_FRAME_DATA_SPHERICAL }, >>> + { AV_PKT_DATA_STEREO3D, AV_FRAME_DATA_STEREO3D }, >>> + { AV_PKT_DATA_AUDIO_SERVICE_TYPE, >>> AV_FRAME_DATA_AUDIO_SERVICE_TYPE }, >>> + { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, >>> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA }, >>> + { AV_PKT_DATA_CONTENT_LIGHT_LEVEL, >>> AV_FRAME_DATA_CONTENT_LIGHT_LEVEL }, >>> + { AV_PKT_DATA_ICC_PROFILE, >>> AV_FRAME_DATA_ICC_PROFILE }, >>> + { AV_PKT_DATA_DYNAMIC_HDR10_PLUS, >>> AV_FRAME_DATA_DYNAMIC_HDR_PLUS }, >>> +}; >>> + >>> int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx, >>> AVFrame *frame, const AVPacket >>> *pkt) >>> { >>> @@ -1429,18 +1443,10 @@ int ff_decode_frame_props_from_pkt(const >>> AVCodecContext *avctx, >>> enum AVPacketSideDataType packet; >>> enum AVFrameSideDataType frame; >>> } sd[] = { >>> - { AV_PKT_DATA_REPLAYGAIN , >>> AV_FRAME_DATA_REPLAYGAIN }, >>> { AV_PKT_DATA_DISPLAYMATRIX, >>> AV_FRAME_DATA_DISPLAYMATRIX }, >> >> Why are you leaving displaymatrix out? > > See the code patch 10/11 removes. Display matrix in global side data is > apparently not meant to be propagated to frames. If i move this line to > the global map, a couple tests change their output due to new side data > in frames. > If you want that to happen (After all, the CLI is not the only library > user), I'd rather remove the skipping code in the CLI first, with the > relevant test refs changes, so this patch is not the one to change them. Actually, both tests use ffprobe, which never injected stream side data into packets before this set, so the ffmpeg code removed in 10/11 makes no difference. Adding DISPLAYMATRIX to the global map here will still mean ffmpeg.c will pass stream side data to decoded frames, though, even if no test covers it. So it would be nice to know why was it being skipped. > >> >> Also, what happens if the same side data is present at both global and >> packet level? Won't you get two instances in the frame? >> I think the correct behaviour would be that packet overrides global. > > I'm keeping the behavior from before this set intact with this at > Andreas' request. > In a previous iteration i ensured packet side data would replace global > side data in the output frames, but Andreas asked me to not do that, as > a decoder could add that same side data type and it should have priority > over container level side data (All entries will be added to the frame > anyway).
Quoting James Almer (2023-10-04 05:22:38) > > Actually, both tests use ffprobe, which never injected stream side data > into packets before this set, so the ffmpeg code removed in 10/11 makes > no difference. > > Adding DISPLAYMATRIX to the global map here will still mean ffmpeg.c > will pass stream side data to decoded frames, though, even if no test > covers it. So it would be nice to know why was it being skipped. Honestly I doubt there's a good reason, the handling of displaymatrix in ffmpeg CLI is a convoluted mess.
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index a7196b5740..3b4bb70689 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1422,6 +1422,20 @@ static int add_metadata_from_side_data(const AVPacket *avpkt, AVFrame *frame) return av_packet_unpack_dictionary(side_metadata, size, frame_md); } +static const struct { + enum AVPacketSideDataType packet; + enum AVFrameSideDataType frame; +} sd_global_map[] = { + { AV_PKT_DATA_REPLAYGAIN , AV_FRAME_DATA_REPLAYGAIN }, + { AV_PKT_DATA_SPHERICAL, AV_FRAME_DATA_SPHERICAL }, + { AV_PKT_DATA_STEREO3D, AV_FRAME_DATA_STEREO3D }, + { AV_PKT_DATA_AUDIO_SERVICE_TYPE, AV_FRAME_DATA_AUDIO_SERVICE_TYPE }, + { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA }, + { AV_PKT_DATA_CONTENT_LIGHT_LEVEL, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL }, + { AV_PKT_DATA_ICC_PROFILE, AV_FRAME_DATA_ICC_PROFILE }, + { AV_PKT_DATA_DYNAMIC_HDR10_PLUS, AV_FRAME_DATA_DYNAMIC_HDR_PLUS }, +}; + int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx, AVFrame *frame, const AVPacket *pkt) { @@ -1429,18 +1443,10 @@ int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx, enum AVPacketSideDataType packet; enum AVFrameSideDataType frame; } sd[] = { - { AV_PKT_DATA_REPLAYGAIN , AV_FRAME_DATA_REPLAYGAIN }, { AV_PKT_DATA_DISPLAYMATRIX, AV_FRAME_DATA_DISPLAYMATRIX }, - { AV_PKT_DATA_SPHERICAL, AV_FRAME_DATA_SPHERICAL }, - { AV_PKT_DATA_STEREO3D, AV_FRAME_DATA_STEREO3D }, - { AV_PKT_DATA_AUDIO_SERVICE_TYPE, AV_FRAME_DATA_AUDIO_SERVICE_TYPE }, - { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA }, - { AV_PKT_DATA_CONTENT_LIGHT_LEVEL, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL }, { AV_PKT_DATA_A53_CC, AV_FRAME_DATA_A53_CC }, { AV_PKT_DATA_AFD, AV_FRAME_DATA_AFD }, - { AV_PKT_DATA_ICC_PROFILE, AV_FRAME_DATA_ICC_PROFILE }, { AV_PKT_DATA_S12M_TIMECODE, AV_FRAME_DATA_S12M_TIMECODE }, - { AV_PKT_DATA_DYNAMIC_HDR10_PLUS, AV_FRAME_DATA_DYNAMIC_HDR_PLUS }, { AV_PKT_DATA_SKIP_SAMPLES, AV_FRAME_DATA_SKIP_SAMPLES }, }; @@ -1453,6 +1459,18 @@ FF_DISABLE_DEPRECATION_WARNINGS FF_ENABLE_DEPRECATION_WARNINGS #endif + for (int i = 0; i < FF_ARRAY_ELEMS(sd_global_map); i++) { + size_t size; + const uint8_t *packet_sd = av_packet_get_side_data(pkt, sd_global_map[i].packet, &size); + if (packet_sd) { + AVFrameSideData *frame_sd; + + frame_sd = av_frame_new_side_data(frame, sd_global_map[i].frame, size); + if (!frame_sd) + return AVERROR(ENOMEM); + memcpy(frame_sd->data, packet_sd, size); + } + } for (int i = 0; i < FF_ARRAY_ELEMS(sd); i++) { size_t size; uint8_t *packet_sd = av_packet_get_side_data(pkt, sd[i].packet, &size); @@ -1489,6 +1507,20 @@ int ff_decode_frame_props(AVCodecContext *avctx, AVFrame *frame) const AVPacket *pkt = avctx->internal->last_pkt_props; int ret; + for (int i = 0; i < FF_ARRAY_ELEMS(sd_global_map); i++) { + const AVPacketSideData *packet_sd = ff_get_coded_side_data(avctx, + sd_global_map[i].packet); + if (packet_sd) { + AVFrameSideData *frame_sd = av_frame_new_side_data(frame, + sd_global_map[i].frame, + packet_sd->size); + if (!frame_sd) + return AVERROR(ENOMEM); + + memcpy(frame_sd->data, packet_sd->data, packet_sd->size); + } + } + if (!(ffcodec(avctx->codec)->caps_internal & FF_CODEC_CAP_SETS_FRAME_PROPS)) { ret = ff_decode_frame_props_from_pkt(avctx, frame, pkt); if (ret < 0)
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/decode.c | 48 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-)