[FFmpeg-devel] [PATCH] h264 bitstream filter

[Måns Rullgård]

Benoit Fouet <benoit.fouet at purplelabs.com> writes:

> Michael Niedermayer wrote:
>> Hi
>>
>> On Thu, Aug 30, 2007 at 05:12:09PM +0200, Benoit Fouet wrote:
>> [...]
>>   
>>> updated patch attached
>>>     
>> [...]
>>   
>>> +            bsfc->priv_data = out;
>>> +            bsfc->filter->priv_data_size = priv_data_size;
>>>     
>>
>> bsfc->filter is shared between all instances
>> writing to it is absolutely not allowed
>>
>> please properly initalize priv_data_size to the size of your context
>> and store whatever you need in this context
>>
>>   
>
> it's always easier after some sleep :)
> here it is, along with other modifications i thought of...
>
> Index: libavcodec/Makefile
> ===================================================================
> --- libavcodec/Makefile	(revision 10260)
> +++ libavcodec/Makefile	(working copy)
> @@ -326,6 +326,7 @@
>  OBJS-$(CONFIG_MP3_HEADER_DECOMPRESS_BSF) += mp3_header_decompress_bsf.o mpegaudiodata.o
>  OBJS-$(CONFIG_MJPEGA_DUMP_HEADER_BSF)  += mjpega_dump_header_bsf.o
>  OBJS-$(CONFIG_IMX_DUMP_HEADER_BSF)     += imx_dump_header_bsf.o
> +OBJS-$(CONFIG_MP4_AVC_TO_AVC_ES_BSF)   += mp4_avc_to_avc_es_bsf.o

Diego sorted these so you'll need to update this.

>  OBJS-$(HAVE_PTHREADS)                  += pthread.o
>  OBJS-$(HAVE_W32THREADS)                += w32thread.o
> Index: libavcodec/allcodecs.c
> ===================================================================
> --- libavcodec/allcodecs.c	(revision 10260)
> +++ libavcodec/allcodecs.c	(working copy)
> @@ -284,5 +284,6 @@
>      REGISTER_BSF    (MP3_HEADER_DECOMPRESS, mp3_header_decompress);
>      REGISTER_BSF    (MJPEGA_DUMP_HEADER, mjpega_dump_header);
>      REGISTER_BSF    (IMX_DUMP_HEADER, imx_dump_header);
> +    REGISTER_BSF    (MP4_AVC_TO_AVC_ES, mp4_avc_to_avc_es);
>  }

Ditto.

> Index: libavcodec/allcodecs.h
> ===================================================================
> --- libavcodec/allcodecs.h	(revision 10260)
> +++ libavcodec/allcodecs.h	(working copy)
> @@ -313,5 +313,6 @@
>  extern AVBitStreamFilter mp3_header_decompress_bsf;
>  extern AVBitStreamFilter mjpega_dump_header_bsf;
>  extern AVBitStreamFilter imx_dump_header_bsf;
> +extern AVBitStreamFilter mp4_avc_to_avc_es_bsf;

Ditto.

> +static void alloc_and_copy( uint8_t **poutbuf,  int *poutbuf_size,
> +                            const uint8_t *in1, uint32_t in1_size,
> +                            const uint8_t *in2, uint32_t in2_size,
> +                            uint32_t nal_offset) {
> +    *poutbuf = av_malloc(in1_size+in2_size);
> +    *poutbuf_size = in1_size+in2_size;
> +    memcpy(*poutbuf, in1, in1_size);
> +    if (in2)
> +        memcpy(*poutbuf+in1_size, in2, in2_size);
> +    (*poutbuf)[nal_offset  ] = 0;
> +    (*poutbuf)[nal_offset+1] = 0;
> +    (*poutbuf)[nal_offset+2] = 0;
> +    (*poutbuf)[nal_offset+3] = 1;

Maybe use AV_WB32 there.

> +}
> +
> +static int mp4_avc_to_avc_es( AVBitStreamFilterContext *bsfc,
> +                              AVCodecContext *avctx, const char *args,
> +                              uint8_t  **poutbuf, int *poutbuf_size,
> +                              const uint8_t *buf, int      buf_size,
> +                              int keyframe ) {
> +    /* nothing to filter */
> +    if (!avctx->extradata || buf_size < 5 || avctx->extradata_size < 9) {
> +        *poutbuf = (uint8_t*) buf;
> +        *poutbuf_size = buf_size;
> +        return 0;
> +    }
> +
> +    else {

No need for the else when you return in the if.  Save yourself a level
of indentation.

> +        AVCBSFContext *ctx = bsfc->priv_data;
> +        uint8_t unit_type = buf[4] & 0x1f;
> +
> +        if (!ctx->sps_pps_data) {
> +            uint16_t unit_size;
> +            uint32_t total_size = 0;
> +            uint8_t *out = NULL, unit_nb, sps_done = 0;
> +            /* skip everything before number of sps units */
> +            const uint8_t *extradata = avctx->extradata+5;
> +            static const uint8_t nalu_header[4] = {0, 0, 0, 1};
> +
> +            /* retrieve sps and pps unit(s) */
> +            unit_nb = *extradata++ & 0x1f; /* number of sps unit(s) */
> +            if (!unit_nb) {
> +                unit_nb = *extradata++; /* number of pps unit(s) */
> +                sps_done++;
> +            }
> +            while (unit_nb--) {
> +                unit_size = AV_RB16(extradata);
> +                total_size += unit_size+4;
> +                if (extradata+2+unit_size > avctx->extradata+avctx->extradata_size) {
> +                    av_free(out);
> +                    return -1;
> +                }
> +                out = av_realloc(out, total_size);
> +                memcpy(out+total_size-unit_size-4, nalu_header, 4);
> +                memcpy(out+total_size-unit_size,   extradata+2, unit_size);
> +                extradata += 2+unit_size;
> +
> +                if (!unit_nb && !sps_done++)
> +                    unit_nb = *extradata++; /* number of pps unit(s) */
> +            }
> +
> +            ctx->sps_pps_data = out;
> +            ctx->size = total_size;
> +        }
> +
> +        if (unit_type == 5 /* IDR picture */)
> +            /* sps and pps have to be prepended to NAL unit */
> +            alloc_and_copy(poutbuf, poutbuf_size,
> +                           ctx->sps_pps_data, ctx->size,
> +                           buf, buf_size,
> +                           ctx->size);
> +        else if (unit_type == 6 /* SEI */ || unit_type == 9 /* AUD */)
> +            /* sps and pps have to be appended to NAL unit */
> +            alloc_and_copy(poutbuf, poutbuf_size,
> +                           buf, buf_size,
> +                           ctx->sps_pps_data, ctx->size,
> +                           0);

This is wrong.  Maybe you misunderstood what I said about where to
insert SPS and PPS.  They should be added before the first type 5 NAL
unit of an IDR picture, after whatever SEI or AUD units that picture
has.  A multislice IDR picture only needs SPS and PPS before the first
slice, and non-IDR pictures should not have SPS or PPS added.

> +        else
> +            alloc_and_copy(poutbuf, poutbuf_size,
> +                           buf, buf_size,
> +                           NULL, 0,
> +                           0);
> +    }
> +
> +    return 1;
> +}
> +
> +AVBitStreamFilter mp4_avc_to_avc_es_bsf = {
> +    "mp4_avc_to_avc_es",

I don't like that name.

> +    sizeof( AVCBSFContext ),

I'm allergic to spaces inside parenthesis.

> +    mp4_avc_to_avc_es,
> +};
> +

-- 
M?ns Rullg?rd
mans at mansr.com