| Message ID | 20190618005940.197421-1-chcunningham@chromium.org |
|---|---|
| State | New |
| Headers | show |
+James On Mon, Jun 17, 2019 at 6:31 PM Chris Cunningham <chcunningham@chromium.org> wrote: > This behavior was added in 2010 to suport some old (and invalid) ogm > files. > https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94 > > But this makes it possible to change the codec in the later headers, > causing codec to be out of sync with internal avctx (eventually > triggering Abrt). Updating the internal ctx for this degenerate case > was deemed not worth it. See discussion here: > https://patchwork.ffmpeg.org/patch/11983/ > --- > libavformat/oggdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c > index e815f42134..19d77f3107 100644 > --- a/libavformat/oggdec.c > +++ b/libavformat/oggdec.c > @@ -545,7 +545,7 @@ static int ogg_packet(AVFormatContext *s, int *sid, > int *dstart, int *dsize, > ogg->curidx = idx; > os->incomplete = 0; > > - if (os->header) { > + if (!ogg->headers) { > if ((ret = os->codec->header(s, idx)) < 0) { > av_log(s, AV_LOG_ERROR, "Header processing failed: %s\n", > av_err2str(ret)); > return ret; > -- > 2.22.0.410.gd8fdbe21b5-goog > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Mon, Jun 17, 2019 at 05:59:40PM -0700, Chris Cunningham wrote: > This behavior was added in 2010 to suport some old (and invalid) ogm > files. https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94 > > But this makes it possible to change the codec in the later headers, > causing codec to be out of sync with internal avctx (eventually > triggering Abrt). Updating the internal ctx for this degenerate case > was deemed not worth it. See discussion here: > https://patchwork.ffmpeg.org/patch/11983/ > --- > libavformat/oggdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c > index e815f42134..19d77f3107 100644 > --- a/libavformat/oggdec.c > +++ b/libavformat/oggdec.c > @@ -545,7 +545,7 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize, > ogg->curidx = idx; > os->incomplete = 0; > > - if (os->header) { > + if (!ogg->headers) { > if ((ret = os->codec->header(s, idx)) < 0) { > av_log(s, AV_LOG_ERROR, "Header processing failed: %s\n", av_err2str(ret)); > return ret; breaks: ./ffmpeg -i bgc.sub.dub.ogm -vframes 3 -y test.webm sample: http://samples.mplayerhq.hu/ogg/bgc.sub.dub.ogm [...]
On Wed, Jun 19, 2019 at 11:25 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > breaks: > ./ffmpeg -i bgc.sub.dub.ogm -vframes 3 -y test.webm > sample: http://samples.mplayerhq.hu/ogg/bgc.sub.dub.ogm > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > Thanks Michael. That file is example of the "invalid" sort we were discussing previously. As I understand it, the spec requires that data packets not be interleaved with header packets - the headers for all streams should conclude before data packets begin. In this file we see a few headers followed by a data packet (stream 0), followed by more headers for streams 1 - 3. We knew this change would break such files. Can we live it? James, any workaround? If not I'm still open to setting st->internal->need_context_update as discussed in the earlier patch (https://patchwork.ffmpeg.org/patch/11983/) Chris
On Wed, Jun 19, 2019 at 7:11 PM Chris Cunningham <chcunningham@chromium.org> wrote: > On Wed, Jun 19, 2019 at 11:25 AM Michael Niedermayer > <michael@niedermayer.cc> wrote: > >> breaks: >> ./ffmpeg -i bgc.sub.dub.ogm -vframes 3 -y test.webm >> sample: http://samples.mplayerhq.hu/ogg/bgc.sub.dub.ogm >> >> [...] >> >> -- >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >> > > Thanks Michael. > > That file is example of the "invalid" sort we were discussing previously. > As I understand it, the spec requires that data packets not be interleaved > with header packets - the headers for all streams should conclude before > data packets begin. In this file we see a few headers followed by a data > packet (stream 0), followed by more headers for streams 1 - 3. > > We knew this change would break such files. Can we live it? James, any > workaround? If not I'm still open to > setting st->internal->need_context_update as discussed in the earlier patch > (https://patchwork.ffmpeg.org/patch/11983/) > > Chris > Quick refresher on my fuzzer input avformat_find_stream_info finds 3 streams [0] AV_CODEC_ID_OPUS [1] AV_CODEC_ID_TEXT [2] AV_CODEC_ID_NONE A bit later we're reading frames out and stream 2 encounters a header that changes the codec from NONE -> AV_CODEC_ID_GSM_MS. ogm_header() assigns this to codecpar->codec_id, but st->internal->avctx->codec_id is never updated (remains NONE). This mismatch ultimately triggers an assert0 in gsm_parse (expecting only gsm codecs). Things tried so far: - keep codec internal in sync (need_context_update = 1) https://patchwork.ffmpeg.org/patch/11983/ - don't allow codec id = none in ogm parse https://patchwork.ffmpeg.org/patch/13529/ - and disallow headers after data (this thread) I think this last one misses the mark slightly. It happens that the a codec change corresponds with a header that comes after data packets start, but I think it could just as easily have occurred without that interleaved data packet. The root question is whether there is some way to know "this header is garbage" ... but a header that assigns a reasonable gsm codec (particularly when the codec was not yet known) seems pretty reasonable.
On 6/19/2019 11:11 PM, Chris Cunningham wrote: > On Wed, Jun 19, 2019 at 11:25 AM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > breaks: > ./ffmpeg -i bgc.sub.dub.ogm -vframes 3 -y test.webm > sample: http://samples.mplayerhq.hu/ogg/bgc.sub.dub.ogm > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > Thanks Michael. > > That file is example of the "invalid" sort we were discussing > previously. As I understand it, the spec requires that data packets not > be interleaved with header packets - the headers for all streams should > conclude before data packets begin. In this file we see a few headers > followed by a data packet (stream 0), followed by more headers for > streams 1 - 3. > > We knew this change would break such files. Can we live it? James, any > workaround? If not I'm still open to > setting st->internal->need_context_update as discussed in the earlier > patch (https://patchwork.ffmpeg.org/patch/11983/) I'm fine going the need_context_update route if trying to skip extra headers is being so problematic. > > Chris
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c index e815f42134..19d77f3107 100644 --- a/libavformat/oggdec.c +++ b/libavformat/oggdec.c @@ -545,7 +545,7 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize, ogg->curidx = idx; os->incomplete = 0; - if (os->header) { + if (!ogg->headers) { if ((ret = os->codec->header(s, idx)) < 0) { av_log(s, AV_LOG_ERROR, "Header processing failed: %s\n", av_err2str(ret)); return ret;