[FFmpeg-devel] [PATCH] lavc: reset codec on receiving packet after EOF in compat_decode

[Michael Niedermayer]

On Sun, Nov 26, 2017 at 12:09:35PM -0300, James Almer wrote:
> On 11/21/2017 6:48 PM, Marton Balint wrote:
> > 
> > 
> > On Thu, 9 Nov 2017, James Cowgill wrote:
> > 
> >> Hi,
> >>
> >> On 09/11/17 14:02, Hendrik Leppkes wrote:
> >>> On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill at debian.org>
> >>> wrote:
> >>>> In commit 061a0c14bb57 ("decode: restructure the core decoding
> >>>> code"), the
> >>>> deprecated avcodec_decode_* APIs were reworked so that they called
> >>>> into the
> >>>> new avcodec_send_packet / avcodec_receive_frame API. This had the
> >>>> side effect
> >>>> of prohibiting sending new packets containing data after a drain
> >>>> packet, but in previous versions of FFmpeg this "worked" and some
> >>>> applications relied on it.
> >>>>
> >>>> To restore some compatibility, reset the codec if we receive a new
> >>>> non-drain
> >>>> packet using the old API after draining has completed. While this does
> >>>> not give the same behaviour as the old API did, in the majority of
> >>>> cases
> >>>> it works and it does not require changes to any other part of the
> >>>> decoding
> >>>> code.
> >>>>
> >>>> Fixes ticket #6775
> >>>> Signed-off-by: James Cowgill <jcowgill at debian.org>
> >>>> ---
> >>>>  libavcodec/decode.c | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> >>>> index 86fe5aef52..2f1932fa85 100644
> >>>> --- a/libavcodec/decode.c
> >>>> +++ b/libavcodec/decode.c
> >>>> @@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx,
> >>>> AVFrame *frame,
> >>>>
> >>>>      av_assert0(avci->compat_decode_consumed == 0);
> >>>>
> >>>> +    if (avci->draining_done && pkt && pkt->size != 0) {
> >>>> +        av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after
> >>>> EOF\n");
> >>>> +        avcodec_flush_buffers(avctx);
> >>>> +    }
> >>>> +
> >>>
> >>> I don't think this is a good idea. Draining and not flushing
> >>> afterwards is a bug in the calling code, and even before recent
> >>> changes it would result in inconsistent behavior and even crashes
> >>> (with select decoders).
> >>
> >> I am fully aware that this will only trigger if the calling code is
> >> buggy. I am trying to avoid silent breakage of those applications doing
> >> this when upgrading to ffmpeg 3.4.
> >>
> >> I was looking at the documentation of avcodec_decode_* recently because
> >> of this and I had some trouble deciding if using the API this way was
> >> incorrect. I expect the downstreams affected thought that what they were
> >> doing was fine and then got angry when ffmpeg suddenly "broke" their
> >> code. This patch at least allows some sort of "transitional period"
> >> until downstreams update.
> > 
> > I think the intent was to flush the codec by passing the NULL packets to
> > it, so it makes a lot of sense to actually do that. Especially since by
> > implicitly doing a flush, we can avoid the undefined behaviour/crashes
> > on the codec side.
> > 
> > Also this is only compatibility code, which probably will be removed at
> > the next bump, I see no harm in making it as compatible as realistically
> > possible.
> 
> The old decode API is not scheduled for removal right now probably
> because 99% of decoders need to be ported.
> This compat code was written so the old API becomes a wrapper for the
> new rather than the other way around, as it was up to 3.3. Supposedly a
> good portion of the versatility of the new API would be handicapped
> otherwise.
> 

> Personally, I think this should be left as is. It is a good incentive
> for downstream to migrate to the new API, as they technically were
> misusing the old API to begin with.

providing compatibility support for an old API that does not actually
work with how applications used the old API is something a tad bit
bizzare

We want to minimize the work everyone has to do.
The more time people have, the more they can spend on improving free
software.

If the old API is going to be removed, any work people have to do
to hunt and track implementation changes in our old API the more
they have wasted time.
If you want people to spend their time on the new API, then you
should not introduce issues in the old API that they need to
workaround
They that way just lost time (debug, fix, test) they could have spend
on the new API or on anything else



> Between fixing their old API usage and migrating, the choice should be
> obvious.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20171127/14e4cbde/attachment.sig>