diff mbox series

[FFmpeg-devel,08/10] avcodec/decode: check for global side data in AVCodecContext side data

Message ID 20230906174431.45558-9-jamrial@gmail.com
State New
Headers show
Series AVCodecContext and AVCodecParameters side data | expand

Commit Message

James Almer Sept. 6, 2023, 5:44 p.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/decode.c | 55 +++++++++++++++++++++++++++++++++++++--------
 libavcodec/decode.h |  2 +-
 2 files changed, 47 insertions(+), 10 deletions(-)

Comments

Andreas Rheinhardt Sept. 11, 2023, 6:35 p.m. UTC | #1 
James Almer:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/decode.c | 55 +++++++++++++++++++++++++++++++++++++--------
>  libavcodec/decode.h |  2 +-
>  2 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 169ee79acd..1a431ba7d7 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1409,25 +1409,31 @@ static int add_metadata_from_side_data(const AVPacket *avpkt, AVFrame *frame)
>      return av_packet_unpack_dictionary(side_metadata, size, frame_md);
>  }
>  
> -int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx,
> +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(AVCodecContext *avctx,
>                                     AVFrame *frame, const AVPacket *pkt)
>  {
>      static const struct {
>          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 },
>      };
>  
> @@ -1440,6 +1446,23 @@ FF_DISABLE_DEPRECATION_WARNINGS
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
>  
> +    for (int i = 0; i < FF_ARRAY_ELEMS(sd_global_map); i++) {
> +        size_t size;
> +        uint8_t *packet_sd = av_packet_get_side_data(pkt, sd_global_map[i].packet, &size);
> +        if (packet_sd) {
> +            AVFrameSideData *frame_sd;
> +
> +            av_log(avctx, AV_LOG_DEBUG,
> +                   "Overwriting global side data of type \"%s\" in output frame with side data "
> +                   "from input packet.\n", av_packet_side_data_name(sd_global_map[i].packet));

Overwriting? Is it guaranteed that the frame has the side data at all?
And if it had, why would you overwrite it at all? After all, if the
decoder added something, isn't this to be preferred to stuff from
AVPacket side data?
Apart from that: packet_sd should be const.

> +
> +            av_frame_remove_side_data(frame, sd_global_map[i].frame);
> +            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);
> @@ -1476,6 +1499,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)
> diff --git a/libavcodec/decode.h b/libavcodec/decode.h
> index a52152e4a7..52e5aafc34 100644
> --- a/libavcodec/decode.h
> +++ b/libavcodec/decode.h
> @@ -67,7 +67,7 @@ int ff_decode_get_packet(AVCodecContext *avctx, AVPacket *pkt);
>  /**
>   * Set various frame properties from the provided packet.
>   */
> -int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx,
> +int ff_decode_frame_props_from_pkt(AVCodecContext *avctx,
>                                     AVFrame *frame, const AVPacket *pkt);
>  
>  /**
James Almer Sept. 11, 2023, 6:41 p.m. UTC | #2 
On 9/11/2023 3:35 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/decode.c | 55 +++++++++++++++++++++++++++++++++++++--------
>>   libavcodec/decode.h |  2 +-
>>   2 files changed, 47 insertions(+), 10 deletions(-)
>>
>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>> index 169ee79acd..1a431ba7d7 100644
>> --- a/libavcodec/decode.c
>> +++ b/libavcodec/decode.c
>> @@ -1409,25 +1409,31 @@ static int add_metadata_from_side_data(const AVPacket *avpkt, AVFrame *frame)
>>       return av_packet_unpack_dictionary(side_metadata, size, frame_md);
>>   }
>>   
>> -int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx,
>> +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(AVCodecContext *avctx,
>>                                      AVFrame *frame, const AVPacket *pkt)
>>   {
>>       static const struct {
>>           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 },
>>       };
>>   
>> @@ -1440,6 +1446,23 @@ FF_DISABLE_DEPRECATION_WARNINGS
>>   FF_ENABLE_DEPRECATION_WARNINGS
>>   #endif
>>   
>> +    for (int i = 0; i < FF_ARRAY_ELEMS(sd_global_map); i++) {
>> +        size_t size;
>> +        uint8_t *packet_sd = av_packet_get_side_data(pkt, sd_global_map[i].packet, &size);
>> +        if (packet_sd) {
>> +            AVFrameSideData *frame_sd;
>> +
>> +            av_log(avctx, AV_LOG_DEBUG,
>> +                   "Overwriting global side data of type \"%s\" in output frame with side data "
>> +                   "from input packet.\n", av_packet_side_data_name(sd_global_map[i].packet));
> 
> Overwriting? Is it guaranteed that the frame has the side data at all?
> And if it had, why would you overwrite it at all? After all, if the
> decoder added something, isn't this to be preferred to stuff from
> AVPacket side data?

I guess you're right, it probably shouldn't.

I'll remove the av_frame_remove_side_data() call bellow as well as the 
debug log, so the behavior will remain the same in such scenarios as 
it's right now (Packet side data will be added to the frame as a second 
entry for the same type, which is normally never used as the _get() 
helper wont reach it).

> Apart from that: packet_sd should be const.

Ok.

> 
>> +
>> +            av_frame_remove_side_data(frame, sd_global_map[i].frame);
>> +            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);
>> @@ -1476,6 +1499,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)
>> diff --git a/libavcodec/decode.h b/libavcodec/decode.h
>> index a52152e4a7..52e5aafc34 100644
>> --- a/libavcodec/decode.h
>> +++ b/libavcodec/decode.h
>> @@ -67,7 +67,7 @@ int ff_decode_get_packet(AVCodecContext *avctx, AVPacket *pkt);
>>   /**
>>    * Set various frame properties from the provided packet.
>>    */
>> -int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx,
>> +int ff_decode_frame_props_from_pkt(AVCodecContext *avctx,
>>                                      AVFrame *frame, const AVPacket *pkt);
>>   
>>   /**
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 169ee79acd..1a431ba7d7 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -1409,25 +1409,31 @@  static int add_metadata_from_side_data(const AVPacket *avpkt, AVFrame *frame)
     return av_packet_unpack_dictionary(side_metadata, size, frame_md);
 }
 
-int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx,
+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(AVCodecContext *avctx,
                                    AVFrame *frame, const AVPacket *pkt)
 {
     static const struct {
         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 },
     };
 
@@ -1440,6 +1446,23 @@  FF_DISABLE_DEPRECATION_WARNINGS
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
 
+    for (int i = 0; i < FF_ARRAY_ELEMS(sd_global_map); i++) {
+        size_t size;
+        uint8_t *packet_sd = av_packet_get_side_data(pkt, sd_global_map[i].packet, &size);
+        if (packet_sd) {
+            AVFrameSideData *frame_sd;
+
+            av_log(avctx, AV_LOG_DEBUG,
+                   "Overwriting global side data of type \"%s\" in output frame with side data "
+                   "from input packet.\n", av_packet_side_data_name(sd_global_map[i].packet));
+
+            av_frame_remove_side_data(frame, sd_global_map[i].frame);
+            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);
@@ -1476,6 +1499,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)
diff --git a/libavcodec/decode.h b/libavcodec/decode.h
index a52152e4a7..52e5aafc34 100644
--- a/libavcodec/decode.h
+++ b/libavcodec/decode.h
@@ -67,7 +67,7 @@  int ff_decode_get_packet(AVCodecContext *avctx, AVPacket *pkt);
 /**
  * Set various frame properties from the provided packet.
  */
-int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx,
+int ff_decode_frame_props_from_pkt(AVCodecContext *avctx,
                                    AVFrame *frame, const AVPacket *pkt);
 
 /**