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

[wm4]

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?

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