[FFmpeg-devel] [PATCH] h264 bitstream filter

[Benoit Fouet]

M?ns Rullg?rd wrote:
> 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.
>
>   

all fixed.

>> +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.
>
>   

i'll have a look

>> +}
>> +
>> +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.
>
>   

true, fixed

>> +        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.

it seems so...

>   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.
>
>   

i'm not sure i get it, here is what i understood, please correct me if
i'm wrong
i receive NALU in the filter
if it's the first coded slice of an IDR picture (type 5 NAL), i prepend
sps and pps NALUs (i don't really have to care about SEI or AUD in the
bitstream filter case, right?)
else, i don't add sps and pps

this would give, in pseudo code:

if (not sps and pps data)
  retrieve sps and pps
  first_idr=1

if (first_idr && nal_unit_type == 5)
  prepend sps and pps
  first_idr=0
else
  don't prepend sps and pps
  if( nal_unit_type != 5)
    first_idr=1

is this right ?

>> +        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.
>
>   

neither do i, i'm open if someone has a better one...

>> +    sizeof( AVCBSFContext ),
>>     
>
> I'm allergic to spaces inside parenthesis.
>
>   

fixed locally

-- 
Ben
Purple Labs S.A.
www.purplelabs.com