[FFmpeg-devel] [PATCH] movenc.c fix (Was: Can av_write_frame() modify pkt.data?)

[Baptiste Coudurier]

Aurelien Jacobs wrote:
> Baptiste Coudurier wrote:
> 
>> Hi,
>>
>> Aurelien Jacobs wrote:
>>> Luca Abeni wrote:
>>>
>>>> Luca Abeni wrote: [...]
>>>>> I'll test h.264 (encoded through libavcodec + x264 with default
>>>>> options) in mp4, mov, matroska, and flv. Is this enough, or
>>>>> should I add some more tests?
>>>> Ok, here are the two possible patches (BTW, I just realized that
>>>> the first one looked smaller because it only fixed the mov muxer,
>>>> and did not fix the flv and mkv muxers...).
>>> The patch passing a ByteIOContext to ff_avc_parse_nal_units() is
>>> better IMO. It avoid one malloc/copy step for every packet.
>>>
>> True, but IMHO it is one step away from moving this code into a
>> bitstream filter.
> 
> Maybe yes. But this function is pretty simple, and converting it to
> a bitstream filter should be simple anyway.

Why not doing it then, instead of wasting time to convert it to
ByteIOContext which will be removed soon, since it is simple.

> This biggest problem will be to auto-insert the bitstream filter
> when it is needed.

Michael suggested a way in -cvslog IIRC.

>> Also this code could not be used also as standalone. Yes, I know this is
>> not intended since not part of public API, but there are some very
>> useful, at least to me, functions into FFmpeg codebase
>> (ff_find_start_code being one, this one will too).
> 
> I'm not sure what you mean by standalone (it's current version already
> depends on ByteIOContext), but anyway, point taken.

I can call ff_avc_parse_nal_units from my own external binary because I
need to parse them for annexb h264 by example.

>> Also patch using ByteIOContext more complex IMHO.
> 
> Not that much complex. And the resulting code is simpler and faster.
> I reviewed the patch carefully, and it is safe and clean.

Resulting code is more complex using ByteIOContext IMHO, and being fast
is not the main point of a muxer.

> Now have a look at the updated attached patch. It simply splits
> ff_avc_parse_nal_units into 2 functions. One doing the basic work
> with a provided ByteIOContext, and the other encapsulating the
> call with the necessary dyn_buf so that caller don't need to
> mess with a ByteIOContext when it don't already have one.

Here we have one useless wrapper func IMHO.

> It should address all your remarks. It don't move away from converting
> to a bitstream filter and it's still usable standalone (as much as
> current code is).
> It also retains advantages of original Luca's patch. It avoids useless
> malloc/copy for each packet, and thus it's faster.
> 

Im suggesting that if any more time is going to be wasted on _coding_
related to this issue, the person should waste it on moving code to a
bitstream filter.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc.                                http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA