| Message ID | 37e48e5d-d5bc-e321-ed93-b37759c472e8@googlemail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 22.10.2016 01:16, Andreas Cadhalpun wrote: > From 9de87a4fb2c6c6311a11a2da5de8554a71adfa66 Mon Sep 17 00:00:00 2001 > From: Andreas Cadhalpun <Andreas.Cadhalpun@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@googlemail.com> > --- > libavformat/utils.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) Ping. (I'd like to have this in 3.2.) Best regards, Andreas
On Sat, Oct 22, 2016 at 01:16:00AM +0200, Andreas Cadhalpun wrote: > On 22.10.2016 00:18, Michael Niedermayer wrote: > > On Mon, Oct 17, 2016 at 08:49:23PM +0200, Andreas Cadhalpun wrote: > >> 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@googlemail.com> > >> --- > >> libavformat/utils.c | 12 ++++++++++++ > >> 1 file changed, 12 insertions(+) > > > > This changes the audio output from > > http://samples.ffmpeg.org/MPEG2/vid_0x80.ts > > > > is that intended ? > > Nice catch: it shouldn't change, of course. > > While processing that sample only the codec_tag gets changed, which is no reason to > close the parser. That is only necessary, when the codec_id changes. > > Fixed patch is attached. > > Best regards, > Andreas > > 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@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@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 the patch seems fine otherwise [...]
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@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@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. For example the ac3 parser stores the codec_id in the parser context with: if(hdr.bitstream_id>10) hdr_info->codec_id = AV_CODEC_ID_EAC3; else if (hdr_info->codec_id == AV_CODEC_ID_NONE) hdr_info->codec_id = AV_CODEC_ID_AC3; So if it is first AV_CODEC_ID_EAC3 and then AV_CODEC_ID_AC3, this wouldn't be updated and the parser would change the avctx codec_id back, which seems wrong. > the patch seems fine otherwise OK, pushed. Best regards, Andreas
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@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@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)? both types might be handled by the same decoder so nothing special might be needed "downstream" [...]
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@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@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. > 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. Best regards, Andreas
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@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@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 [...]
On 03.11.2016 11:07, Michael Niedermayer wrote: > On Thu, Nov 03, 2016 at 01:04:21AM +0100, Andreas Cadhalpun wrote: >> 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 ? I meant: C. The demuxer changing the codec_id. > B looks like a violation of the API, the caller cant change things > without reopening the parser Agreed. > 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 Also agreed. > 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 ? I guess the gsm parser, as the blocksize is not updated once it is set, but different between gsm and gsm_ms. >> 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 I tried a few possibly interesting cases, but in no case could I trigger the added calls to av_parser_close for codec_ids of the same parser. Nonetheless, some like mp2/mp3 worked just fine, while e.g. gsm/gsm_ms broke badly. Best regards, Andreas
From 9de87a4fb2c6c6311a11a2da5de8554a71adfa66 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun <Andreas.Cadhalpun@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@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; + } + ret = avcodec_parameters_to_context(st->internal->avctx, st->codecpar); if (ret < 0) return ret; -- 2.9.3