[FFmpeg-devel] FFmpeg 3.4.1

Fri Dec 8 08:30:01 EET 2017

On Fri, 8 Dec 2017 06:52:20 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Dec 08, 2017 at 01:09:50AM -0300, James Almer wrote:
> > On 12/8/2017 12:26 AM, wm4 wrote:  
> > > On Thu, 7 Dec 2017 23:23:51 +0100
> > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > >   
> > >> On Tue, Nov 21, 2017 at 07:58:18PM +0100, Michael Niedermayer wrote:  
> > >>> On Sat, Nov 18, 2017 at 09:11:17PM +0100, Michael Niedermayer wrote:    
> > >>>> On Sat, Nov 18, 2017 at 09:50:33AM +0100, Hendrik Leppkes wrote:    
> > >>>>> On Sat, Nov 18, 2017 at 3:05 AM, Michael Niedermayer
> > >>>>> <michael at niedermayer.cc> wrote:    
> > >>>>>> On Fri, Nov 17, 2017 at 09:55:47AM +0100, Hendrik Leppkes wrote:    
> > >>>>>>> On Fri, Nov 17, 2017 at 5:00 AM, Michael Niedermayer
> > >>>>>>> <michael at niedermayer.cc> wrote:    
> > >>>>>>>> On Thu, Nov 16, 2017 at 01:51:34PM +0100, Carl Eugen Hoyos wrote:    
> > >>>>>>>>> 2017-11-16 13:44 GMT+01:00 Michael Niedermayer <michael at niedermayer.cc>:    
> > >>>>>>>>>> On Thu, Nov 16, 2017 at 01:04:27AM +0100, Carl Eugen Hoyos wrote:    
> > >>>>>>>>>>> 2017-11-15 13:34 GMT+01:00 Michael Niedermayer <michael at niedermayer.cc>:    
> > >>>>>>>>>>>> Hi all
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> I intend to make 3.4.1 very soon    
> > >>>>>>>>>>>
> > >>>>>>>>>>> Shouldn't we first decide on how to proceed with #6775?    
> > >>>>>>>>>>
> > >>>>>>>>>> This would be ideal.
> > >>>>>>>>>>
> > >>>>>>>>>> IIUC this is a regression from bddb2343b6e594e312dadb5d21b408702929ae04    
> > >>>>>>>>>
> > >>>>>>>>> This was confirmed by more than one developer, yes.
> > >>>>>>>>>    
> > >>>>>>>>>> I see a patch that is said to improve 6775, can that be applied and
> > >>>>>>>>>> would that resolve this ?    
> > >>>>>>>>>
> > >>>>>>>>> Iiuc, it would not completely resolve the issue, see:
> > >>>>>>>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=881536
> > >>>>>>>>>    
> > >>>>>>>>>> If so why was it not applied yet ?    
> > >>>>>>>>>
> > >>>>>>>>> The patch did not get support here, see:
> > >>>>>>>>> [FFmpeg-devel] [PATCH] lavc: reset codec on receiving packet after EOF
> > >>>>>>>>> in compat_decode    
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> Is someone working on fixing this better than with the available patch ?
> > >>>>>>>>    
> > >>>>>>>
> > >>>>>>> I don't even agree this needs fixing. Those projects use the API wrong.    
> > >>>>>>
> > >>>>>> Had we documented the correct/wrong use precissely in the past when
> > >>>>>> these wrong uses where written ?
> > >>>>>>
> > >>>>>> Because if it was documented then few should have made the mistake.
> > >>>>>> But it seems this affects multiple projects, so i wonder if our API
> > >>>>>> really excluded the use
> > >>>>>>    
> > >>>>>
> > >>>>> Apparently not well enough, but I also don't even understand why you
> > >>>>> would *want* to drain in the middle of decoding.
> > >>>>>
> > >>>>> The only mention of sending NULL/0 packets (in 3.0 docs, before the
> > >>>>> new API was introduced) do include the "at the end", however.
> > >>>>>
> > >>>>> From CODEC_CAP_DELAY:
> > >>>>>  * Decoders:
> > >>>>>  * The decoder has a non-zero delay and needs to be fed with avpkt->data=NULL,
> > >>>>>  * avpkt->size=0 at the end to get the delayed data until the decoder no longer
> > >>>>>  * returns frames.
> > >>>>>
> > >>>>> From avcodec_decode_video2
> > >>>>> * @note Codecs which have the AV_CODEC_CAP_DELAY capability set have a delay
> > >>>>> * between input and output, these need to be fed with avpkt->data=NULL,
> > >>>>> * avpkt->size=0 at the end to return the remaining frames.
> > >>>>>
> > >>>>> There is a few more mentions of the same concept, but as far as I can
> > >>>>> see all include the key words "at the end".
> > >>>>>     
> > >>>>     
> > >>>>> For the suggested patch, draining and flushing in the middle of a
> > >>>>> bitstream is still going to result in problems, though, since it
> > >>>>> removes all reference frames, for example.
> > >>>>> The original behavior cannot really be stored, which was to just keep
> > >>>>> feeding frames into the decoder after a drain without a flush.
> > >>>>> However, some decoders actually crashed when you did this, so this was
> > >>>>> a rather unsafe action to begin with (not an issue any longer, since
> > >>>>> this pattern is now blocked).    
> > >>>>
> > >>>> Did the previous code really work if the frame after a flush was not a
> > >>>> new keyframe or there was some use of previous references ?    
> > >>>
> > >>> ping
> > >>>
> > >>> so what is the status of this?
> > >>>
> > >>> Ticket 6775 is still open, neither a workaround was applied nor was
> > >>> it closed as invalid. Only one workaround was proposed which was
> > >>> claimed to be worse than the code before.
> > >>> It seems the discussion died down.
> > >>> If theres no activity on this in the next days then i intend to make
> > >>> the release with whats in release/3.4 at the time. I dont think
> > >>> blind waiting will do any good, id rather release early and often ...
> > >>>
> > >>> Also if someone wants to write some release notes about this issue,
> > >>> that is IMO a good idea ...    
> > >>
> > >> So this code is completely unmaintained ?
> > >> Noone cares about pushing a workaround ?
> > >> Noone cares about closing the ticket as wont fix ?
> > >> Noone cares about explaining why neither should be done ?
> > >>
> > >> I intend to make the release tomorrow or as soon as i have time, we
> > >> have waited too long already. I can of course wait more if people want
> > >> but then please have a plan on resolving the issue that the release is
> > >> delayed for  
> > > 
> > > I didn't read all of this, but it's probably due to the API user
> > > passing as null packet, which indeed signals EOF. What happens if you
> > > send more packets after sending EOF has always been undefined behavior,
> > > and some audio decoders could crash if you did that. (It's fine if
> > > you flush the decoder, of course.) I think I brought it up in the past
> > > (and nobody cared), so I added explicit workarounds to my own API usage
> > > code to avoid crashes. Why are you making so much noise around it now?  
> > 
> > When the old decode API was turned into a wrapper for the new, some
> > applications using said API this way started to experience
> > issues/crashes that did not happen before. So basically, a change of
> > behavior which, as you put it, was apparently undefined.
> > 
> > Some argue it should not crash if it didn't before, others argue that it
> > was never meant to be used this way. What Michael wants to know in order
> > to release 3.4.1 is, what should be done? Should it be addressed, or
> > left as is? If the former, do we apply the patch someone proposed in
> > another thread, or something else? If the latter, then the ticket should
> > be closed and the discussion ends there.  
> 
> yes
> also to add to this, 6775 is set to the highest priority (critical)

Also definitely wrong API usage.

> and its our only bug set to that priority.

Since when does the bug tracker mean anything? Bug reports that are
wrt. my project are literally ignored.

> The priority likely makes sense from a distribution packagers
> point of view. And while it might not make sense from our point of
> view to set the priority to that in a vacuum. We should care a bit
> about downstream and what causes significant pain to them.
> 
> that said delaying releases for issues that noone works on is bad
> 
> Thanks
> 
> [...]