[FFmpeg-devel] [PATCH] avformat: close parser if codec changed

[Michael Niedermayer]

On Thu, Nov 03, 2016 at 01:04:21AM +0100, Andreas Cadhalpun wrote:
> On 03.11.2016 00:42, Michael Niedermayer wrote:
> > On Wed, Nov 02, 2016 at 10:30:30PM +0100, Andreas Cadhalpun wrote:
> >> On 02.11.2016 13:07, Michael Niedermayer wrote:
> >>> On Sat, Oct 22, 2016 at 01:16:00AM +0200, Andreas Cadhalpun wrote:
> >>>>  utils.c |   12 ++++++++++++
> >>>>  1 file changed, 12 insertions(+)
> >>>> ffefc22756b774cb7652587207ae66cfbf681be3  0001-avformat-close-parser-if-codec-changed.patch
> >>>> From 9de87a4fb2c6c6311a11a2da5de8554a71adfa66 Mon Sep 17 00:00:00 2001
> >>>> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >>>> Date: Mon, 17 Oct 2016 20:26:51 +0200
> >>>> Subject: [PATCH] avformat: close parser if codec changed
> >>>>
> >>>> The parser depends on the codec and thus must not be used with a different one.
> >>>> If it is, the 'avctx->codec_id == s->parser->codec_ids[0] ...' assert in
> >>>> av_parser_parse2 gets triggered.
> >>>>
> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >>>> ---
> >>>>  libavformat/utils.c | 12 ++++++++++++
> >>>>  1 file changed, 12 insertions(+)
> >>>>
> >>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
> >>>> index 70dbfa1..a8a78ed 100644
> >>>> --- a/libavformat/utils.c
> >>>> +++ b/libavformat/utils.c
> >>>> @@ -480,6 +480,12 @@ static int update_stream_avctx(AVFormatContext *s)
> >>>>          if (!st->internal->need_context_update)
> >>>>              continue;
> >>>>  
> >>>> +        /* close parser, because it depends on the codec */
> >>>> +        if (st->parser && st->internal->avctx->codec_id != st->codecpar->codec_id) {
> >>>> +            av_parser_close(st->parser);
> >>>> +            st->parser = NULL;
> >>>> +        }
> >>>> +
> >>>>          /* update internal codec context, for the parser */
> >>>>          ret = avcodec_parameters_to_context(st->internal->avctx, st->codecpar);
> >>>>          if (ret < 0)
> >>>> @@ -1515,6 +1521,12 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
> >>>>                  st->info->found_decoder = 0;
> >>>>              }
> >>>>  
> >>>> +            /* close parser, because it depends on the codec */
> >>>> +            if (st->parser && st->internal->avctx->codec_id != st->codecpar->codec_id) {
> >>>> +                av_parser_close(st->parser);
> >>>> +                st->parser = NULL;
> >>>> +            }
> >>>> +
> >>>
> >>> what if the codec id differs but both are supported by the parser ?
> >>> AVCodecParser has a list of 5 codec ids ?
> >>>
> >>> I didnt find a testcase where this makes a difference, just wondering
> >>
> >> I think the parser should be re-initialized in that case, too.
> > 
> > doesnt this lead to data loss (parser internal buffers being lost at
> > a transition between 2 types)?
> 
> Yes, but it's not clear that the parser internal state is still correct
> after a change of the codec id.

what exact case are we talking about ?

A. The parser changeing the codec_id ?
B. The caller changing the codec_id ?

B looks like a violation of the API, the caller cant change things
without reopening the parser

for A id consider the parser completely broken if it changes the
codec id and falls in a inconsistent state without the user
detecting that and closing it.
Such requirement for user apps also is not documented

Parsers dont handle unrelated formats, they handle things that are
similar or identical like  AV_CODEC_ID_MJPEG, AV_CODEC_ID_JPEGLS

some parsers dont set the codec_id, like *jpeg doesnt
mpeg audio contains checks to explicitly check for changing codec id
only ac3 and mpeg2 seem to set the codec id at all

which parser becomes inconsistent with itself when changing codec id ?


> 
> > both types might be handled by the same decoder so nothing special
> > might be needed "downstream"
> 
> It might work for some decoders and might not work for others.
> It's hard to tell without samples.

making samples should be as easy as concatenating 2 streams if
someone wants to check what happens exactly

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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20161103/b122d30f/attachment.sig>