[FFmpeg-devel] [PATCH v3 1/3] libavcodec: write out user data unregistered SEI for x264

[Marton Balint]

On Sat, 15 May 2021, Brad Hards wrote:

> Signed-off-by: Brad Hards <bradh at frogmouth.net>
> ---
> libavcodec/libx264.c | 35 ++++++++++++++++++++++++++++++-----
> 1 file changed, 30 insertions(+), 5 deletions(-)

Please designate the name of the touched component in the prefix of 
the commit message, e.g:

libavcodec/libx264: write out user data unregistered SEI

>
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 1c27f7b441..feee8f8ee6 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -31,6 +31,7 @@
> #include "internal.h"
> #include "packet_internal.h"
> #include "atsc_a53.h"
> +#include "sei.h"
>
> #if defined(_MSC_VER)
> #define X264_API_IMPORTS 1
> @@ -303,6 +304,7 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
>     int64_t wallclock = 0;
>     X264Opaque *out_opaque;
>     AVFrameSideData *sd;
> +    int total_unreg_sei = 0;
>
>     x264_picture_init( &x4->pic );
>     x4->pic.img.i_csp   = x4->params.i_csp;
> @@ -316,6 +318,7 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
>     x4->pic.img.i_plane = avfmt2_num_planes(ctx->pix_fmt);
>
>     if (frame) {
> +        void *sei_data_a53_cc;
>         for (i = 0; i < x4->pic.img.i_plane; i++) {
>             x4->pic.img.plane[i]    = frame->data[i];
>             x4->pic.img.i_stride[i] = frame->linesize[i];
> @@ -349,28 +352,50 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
>         reconfig_encoder(ctx, frame);
>
>         if (x4->a53_cc) {
> -            void *sei_data;
>             size_t sei_size;
>
> -            ret = ff_alloc_a53_sei(frame, 0, &sei_data, &sei_size);
> +            ret = ff_alloc_a53_sei(frame, 0, &sei_data_a53_cc, &sei_size);
>             if (ret < 0) {
>                 av_log(ctx, AV_LOG_ERROR, "Not enough memory for closed captions, skipping\n");
> -            } else if (sei_data) {
> +            } else if (sei_data_a53_cc) {
>                 x4->pic.extra_sei.payloads = av_mallocz(sizeof(x4->pic.extra_sei.payloads[0]));
>                 if (x4->pic.extra_sei.payloads == NULL) {
>                     av_log(ctx, AV_LOG_ERROR, "Not enough memory for closed captions, skipping\n");
> -                    av_free(sei_data);
> +                    av_free(sei_data_a53_cc);
>                 } else {
>                     x4->pic.extra_sei.sei_free = av_free;
>
>                     x4->pic.extra_sei.payloads[0].payload_size = sei_size;
> -                    x4->pic.extra_sei.payloads[0].payload = sei_data;
> +                    x4->pic.extra_sei.payloads[0].payload = sei_data_a53_cc;
>                     x4->pic.extra_sei.num_payloads = 1;
>                     x4->pic.extra_sei.payloads[0].payload_type = 4;
>                 }
>             }
>         }
>
> +        for (int j = 0; j < frame->nb_side_data; j++)
> +            if (frame->side_data[j]->type == AV_FRAME_DATA_SEI_UNREGISTERED)
> +                total_unreg_sei++;
> +        if (total_unreg_sei > 0) {
> +            x264_sei_t *sei = &(x4->pic.extra_sei);
> +            sei->payloads = av_realloc_array(sei->payloads,
> +                                             sei->num_payloads + total_unreg_sei,
> +                                             sizeof(x264_sei_payload_t));
> +            if (!sei->payloads) {
> +                av_log(ctx, AV_LOG_ERROR, "Not enough memory for unregistered SEI, skipping\n");

The function should return ENOMEM in case of failure, not conitinue with 
limited functionality. The error message is not needed in that case. Yes, 
I understand there is code which skips functionality if allocation fails, 
but that is wrong, and new code should not do that.

There are similar issues for patch 2 and 3.

Regards,
Marton

> +                av_free(sei_data_a53_cc);
> +                sei->num_payloads = 0;
> +            } else
> +                for (int j = 0; j < frame->nb_side_data; j++)
> +                    if (frame->side_data[j]->type == AV_FRAME_DATA_SEI_UNREGISTERED) {
> +                        x264_sei_payload_t *payload = &(sei->payloads[sei->num_payloads]);
> +                        payload->payload = frame->side_data[j]->data;
> +                        payload->payload_size = frame->side_data[j]->size;
> +                        payload->payload_type = SEI_TYPE_USER_DATA_UNREGISTERED;
> +                        sei->num_payloads++;
> +                    }
> +        }
> +
>         sd = av_frame_get_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST);
>         if (sd) {
>             if (x4->params.rc.i_aq_mode == X264_AQ_NONE) {
> -- 
> 2.27.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>