[FFmpeg-devel] FFmpeg 3.4.1

Fri Dec 8 21:46:51 EET 2017

On Fri, Dec 08, 2017 at 01:25:16PM +0100, Marton Balint wrote:
> 
> 
> On Fri, 8 Dec 2017, wm4 wrote:
> 
> >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.
> 
> IMHO the documentation was not entirely clear. In any case,
> undefined behaviour was changed, which broke two major projects. As
> I mentioned in the other thread, I see no downside of applying the
> patch, therefore it should be applied.

i agree
is anyone still against ?
if not then please apply it
ill wait 24h after that to give time for testing/bug reports and then
make the release as soon as i have time

Thanks alot !

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Take away the freedom of one citizen and you will be jailed, take away
the freedom of all citizens and you will be congratulated by your peers
in Parliament.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171208/f6fc2674/attachment.sig>