[FFmpeg-devel] [FFmpeg-cvslog] libavcodec/libx264: add user data unregistered SEI encoding

[James Almer]

On 10/18/2021 7:38 PM, James Almer wrote:
> On 10/18/2021 7:34 PM, Derek Buitenhuis wrote:
>> On 10/18/2021 11:05 PM, Michael Niedermayer wrote:
>>> This code breaks bitexact mode for some files, i have not looked into 
>>> this
>>> any deeper yet.
>>
>> [23:18] <@jamrial_> Daemon404: that libx264 patch conflicts with the 
>> closed caption code. it overwrites the extra_sei struct and leaks it
>>
>> ^ presumably this is why
> 
> Actually no. It calls av_fast_realloc() on a (potentially) av_malloc'd 
> buffer, which i think is a valid scenario. Also, these samples have no 
> closed captions, so the relevant code is never run.
> 
> The problem i guess is in the following line:
> 
> sei_payload->payload = side_data->data;
> 
> Which should be an av_memdup() or similar, because side_data->data is 
> the frame's side data, and by the time the encoder tries to do something 
> with it, it may have been freed.
> Also, this code should set x4->pic.extra_sei.sei_free to av_free().

The following should fix it

> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 507fee39f2..630dfb3942 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -115,9 +115,6 @@ typedef struct X264Context {
>       * encounter a frame with ROI side data.
>       */
>      int roi_warned;
> -
> -    void *sei_data;
> -    int sei_data_size;
>  } X264Context;
> 
>  static void X264_log(void *p, int level, const char *fmt, va_list args)
> @@ -449,20 +446,24 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
> 
>          for (int j = 0; j < frame->nb_side_data; j++) {
>              AVFrameSideData *side_data = frame->side_data[j];
> +            unsigned int sei_data_size = 0;
>              void *tmp;
>              x264_sei_payload_t *sei_payload;
>              if (side_data->type != AV_FRAME_DATA_SEI_UNREGISTERED)
>                  continue;
> -            tmp = av_fast_realloc(x4->sei_data, &x4->sei_data_size, (sei->num_payloads + 1) * sizeof(*sei_payload));
> +            tmp = av_fast_realloc(sei->payloads, &sei_data_size, (sei->num_payloads + 1) * sizeof(*sei_payload));
>              if (!tmp) {
> -                av_freep(&x4->pic.extra_sei.payloads);
>                  av_freep(&x4->pic.prop.quant_offsets);
>                  return AVERROR(ENOMEM);
>              }
> -            x4->sei_data = tmp;
> -            sei->payloads = x4->sei_data;
> +            sei->payloads = tmp;
> +            sei->sei_free = av_free;
>              sei_payload = &sei->payloads[sei->num_payloads];
> -            sei_payload->payload = side_data->data;
> +            sei_payload->payload = av_memdup(side_data->data, side_data->size);
> +            if (!sei_payload->payload) {
> +                av_freep(&x4->pic.prop.quant_offsets);
> +                return AVERROR(ENOMEM);
> +            }
>              sei_payload->payload_size = side_data->size;
>              sei_payload->payload_type = SEI_TYPE_USER_DATA_UNREGISTERED;
>              sei->num_payloads++;
> @@ -533,8 +534,6 @@ static av_cold int X264_close(AVCodecContext *avctx)
>      x264_param_cleanup(&x4->params);
>  #endif
> 
> -    av_freep(&x4->sei_data);
> -
>      if (x4->enc) {
>          x264_encoder_close(x4->enc);
>          x4->enc = NULL;