[FFmpeg-devel] [PATCH] move avc sps/pps generation in a function in movenc
Fri Jan 11 01:20:26 CET 2008
Aurelien Jacobs wrote:
> Baptiste Coudurier wrote:
>> Aurelien Jacobs wrote:
>>> 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 ?
>>> 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
>> 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
>> 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
> 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
More information about the ffmpeg-devel