[FFmpeg-devel] [PATCH] move avc sps/pps generation in a function in movenc

[Baptiste Coudurier]

Hi,

Aurelien Jacobs wrote:
> Baptiste Coudurier wrote:
> 
>> Hi
>>
>> Aurelien Jacobs wrote:
>>> Hi,
>>>
>>> The attached patch moves the avc sps/pps generation code in a
>>> function. The final goal is to reuse this function from matroskaenc
>>> (and so the next step will obviously be to move this func in its
>>> own file). Is this patch OK ?
>>>
>>> Aurel
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> Index: libavformat/movenc.c
>>> ===================================================================
>>> --- libavformat/movenc.c	(revision 11447)
>>> +++ libavformat/movenc.c	(working copy)
>>> @@ -452,10 +452,10 @@
>>>      return end + 3;
>>>  }
>>>  
>>> -static int avc_parse_nal_units(uint8_t **buf, int *size)
>>> +static int avc_parse_nal_units(uint8_t *buf_in, uint8_t **buf, int
>>> *size)
>> Is changing the prototype really necessary ?
> 
> It could be avoided for movenc. But it's needed if you want the
> source buffer to *not* be de-allocated in some cases (will be
> required in matroskaenc).
> 
>>>  {
>>>      ByteIOContext *pb;
>>> -    uint8_t *p = *buf;
>>> +    uint8_t *p = buf_in;
>>>      uint8_t *end = p + *size;
>>>      uint8_t *nal_start, *nal_end;
>>>      int ret = url_open_dyn_buf(&pb);
>>> @@ -475,22 +475,20 @@
>>>      return 0;
>>>  }
>>>  
>>> -static int mov_write_avcc_tag(ByteIOContext *pb, MOVTrack *track)
>>> +static int avc_write_sps_pps(ByteIOContext *pb, uint8_t *data, int
>>> len)
>> This specific formating was defined by iso and meant to be put in
>> 'avcC' atom in iso media, loosing this information would be sad IMHO,
>> I'd prefer something like isom_write_avcc.
> 
> And this specific formatting is also described in the matroska spec...

I guess pasted from iso specs.

> So I thought that describing what the function does instead of who
> specified it, was more useful.
> But if you insist, I will change the name. I don't care about it.

IMHO avc_write_sps_pps is not adequate due to the specific formatting,
so yes please.

>>> [...]
>>>
>>> +            int ret = avc_parse_nal_units(data, &buf, &len);
>>> +            if (ret < 0)
>>> +                return ret;
>> IMHO unrelated to the patch and there is another occurence which would
>> need to be checked.
> 
> It is somewhat related, as this code is moved into a function returning
> and int, and thus, it's good practice to return error codes.
> But I will commit it separately. This should make things clearer.

Yes, it will be better.

>> [...]
>>
>> av_free(buf) should be more clear since buf was allocated, not data,
>> data is parameter, this is confusing.
>
> The problem is that buf is modified in the meantime, and thus, it
> don't point to the start of the allocated memory anymore. That's
> why a backup pointer to the start of allocated memory (data) is
> needed.

Humm yes.

> So, now, do you think the patch is OK, and do you want me to rename
> the function to isom_write_avcc or not ?

Yes, parts are OK, checking return value for avc_parse_nal_units (both
cases), then to correctly split parts in commits IMHO, extract
isom_write_avcc function, and then change prototype of avc_parse_nal_units.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312