[FFmpeg-devel] [PATCH 1/4] avcodec/avpacket: add av_packet_copy_side_data()

James Almer jamrial at gmail.com
Mon Sep 25 20:51:28 EEST 2017


On 9/25/2017 2:45 PM, wm4 wrote:
> On Mon, 25 Sep 2017 14:37:52 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>> On 9/25/2017 2:29 PM, wm4 wrote:
>>> On Mon, 25 Sep 2017 14:07:54 -0300
>>> James Almer <jamrial at gmail.com> wrote:
>>>   
>>>> On 9/25/2017 1:43 PM, wm4 wrote:  
>>>>> On Mon, 25 Sep 2017 10:58:31 -0300
>>>>> James Almer <jamrial at gmail.com> wrote:
>>>>>     
>>>>>>> Using av_packet_copy_props() instead of introducing yet another weird
>>>>>>> function would be better.      
>>>>>>
>>>>>> It can't be used in some cases. Look at the last two patches in the set.
>>>>>> copy_props can't be used there without some extra pointless work.    
>>>>>
>>>>> Well, you need to copy the props first, before you write any fields
>>>>> that are touched by the function.    
>>>>
>>>> How so? The function only looks at side_data and side_data_size from a
>>>> const src packet, then writes those same two fields to the dst packet
>>>> if copying was successful. av_packet_free_side_data() also only cares
>>>> about those two fields.
>>>>
>>>> I don't know why whoever wrote the code in ffmpeg.c and movenc.c didn't
>>>> use copy_props(), but a quick read hints they didn't want to copy any
>>>> other property except side data, apparently as it would break whatever
>>>> pts/duration calculations they were doing with their tmp packets.  
>>>
>>> So, copy pts/duration after "copying" (ref-ing) the entire packet. I
>>> have to do similar things in my code with AVFrame.  
>>
>> ffmpeg.c in this case doesn't ref the packet. It inits it, writes some
>> fields based on (but without directly copying them from) the source
>> packet, then copies the side data.
>>
>> If you really don't want functions this specific and are willing to fix
>> the two cases in ffmpeg.c and movenc.c to use copy_props then i can
>> repurpose this patchset to only deprecate the existing
>> av_copy_packet_side_data().
>> Keep in mind however that by deprecating and removing said function
>> without adding a direct replacement we'll probably be annoying existing
>> users of said function (Chromium is one it seems) by forcing them to
>> also work around the extra stuff copy_props() would do to their packets.
> 
> What exactly is the trouble of just moving setting pts/duration below
> the function call?

Ask users of the existing function, not me who just wants to get rid of
it for being awful.

> 
> Also adding a function with very similar name and subtly different
> semantics (whose differences aren't even explained) is a very bad idea.

The differences are stated in both the commit message and the doxy:
There will be no leaks and no changes to the dst packet in case of
failure. Otherwise, it's functionally the same.

Again, do you want me to just deprecate the existing function without
adding a direct replacement? I'll do that. But please fix the two
existing uses before the deprecation period is over.


More information about the ffmpeg-devel mailing list