| Message ID | 20190207011303.226684-1-chcunningham@chromium.org |
|---|---|
| State | Accepted |
| Commit | bb11584924d6190a9028cbb319891028f44856a9 |
| Headers | show |
This is a follow up to feedback in http://ffmpeg.org/pipermail/ffmpeg-devel/2019-February/239751.html On Wed, Feb 6, 2019 at 5:13 PM chcunningham <chcunningham@chromium.org> wrote: > Codec information may change while reading ogg packets. Update the > stream's internal avctx to match. > --- > libavformat/oggparseogm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavformat/oggparseogm.c b/libavformat/oggparseogm.c > index a07453760b..b07a5d55ba 100644 > --- a/libavformat/oggparseogm.c > +++ b/libavformat/oggparseogm.c > @@ -114,6 +114,9 @@ ogm_header(AVFormatContext *s, int idx) > bytestream2_get_buffer(&p, st->codecpar->extradata, > st->codecpar->extradata_size); > } > } > + > + // Update internal avctx with changes to codecpar above. > + st->internal->need_context_update = 1; > } else if (bytestream2_peek_byte(&p) == 3) { > bytestream2_skip(&p, 7); > if (bytestream2_get_bytes_left(&p) > 1) > -- > 2.20.1.611.gfbb209baf1-goog > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 2/6/2019 10:13 PM, chcunningham wrote: > Codec information may change while reading ogg packets. Update the > stream's internal avctx to match. > --- > libavformat/oggparseogm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavformat/oggparseogm.c b/libavformat/oggparseogm.c > index a07453760b..b07a5d55ba 100644 > --- a/libavformat/oggparseogm.c > +++ b/libavformat/oggparseogm.c > @@ -114,6 +114,9 @@ ogm_header(AVFormatContext *s, int idx) > bytestream2_get_buffer(&p, st->codecpar->extradata, st->codecpar->extradata_size); > } > } > + > + // Update internal avctx with changes to codecpar above. > + st->internal->need_context_update = 1; > } else if (bytestream2_peek_byte(&p) == 3) { > bytestream2_skip(&p, 7); > if (bytestream2_get_bytes_left(&p) > 1) Can a valid ogm stream contain more than one header packet? Because the issue here as far as i can see is that ogm_header() is not validating the output of ff_codec_get_id() and is happily accepting and propagating AV_CODEC_ID_NONE as derived from it in a late packet, long after the parser and everything else was already initialized. No other ogg module sets st->internal->need_context_update and all of them may also end up calling the respective header reading function more than once on invalid files, but unlike the ogm one, all of them hardcode the codec_id instead of blindly accepting a potentially bogus value derived from the bitstream.
On Wed, Feb 6, 2019 at 6:03 PM James Almer <jamrial@gmail.com> wrote: > Can a valid ogm stream contain more than one header packet? Looking at ogg_packet oggdec.c, we read headers until hitting the first non-header (counted in ogg stream field nb_headers), so multiple headers per stream is probably ok. But multiple codecs in a given stream seems supsect to me. Someone with more ogg expertise should chime in. Because the > issue here as far as i can see is that ogm_header() is not validating > the output of ff_codec_get_id() and is happily accepting and propagating > AV_CODEC_ID_NONE as derived from it in a late packet, long after the > parser and everything else was already initialized. > > No other ogg module sets st->internal->need_context_update and all of > them may also end up calling the respective header reading function more > than once on invalid files, but unlike the ogm one, all of them hardcode > the codec_id instead of blindly accepting a potentially bogus value > derived from the bitstream. > I'm open to hard-coding codec for gsm. One thing I notice is that the codec is just one of several codecpar fields that are potentially changing. If any of those change without need_context_update, it seems like values in the internal avctx could become stale?
Hi
On Wed, Feb 06, 2019 at 05:13:03PM -0800, chcunningham wrote:
> From: chcunningham <chcunningham@chromium.org>
Is the name intended ? As this ends up as author name in git, and several
developers dislike this:
(and i cannot edit other authors names of course, that would be not right)
(from IRC)
<j-b> Could you stop committing things like this?
<j-b> his name is "Chris Cunningham", not "chcunningham"
[...]
I'll re-send with my full name. On Fri, Feb 8, 2019 at 4:57 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > Hi > > On Wed, Feb 06, 2019 at 05:13:03PM -0800, chcunningham wrote: > > From: chcunningham <chcunningham@chromium.org> > > Is the name intended ? As this ends up as author name in git, and several > developers dislike this: > (and i cannot edit other authors names of course, that would be not right) > > (from IRC) > <j-b> Could you stop committing things like this? > <j-b> his name is "Chris Cunningham", not "chcunningham" > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The real ebay dictionary, page 1 > "Used only once" - "Some unspecified defect prevented a second use" > "In good condition" - "Can be repaird by experienced expert" > "As is" - "You wouldnt want it even if you were payed for it, if you knew > ..." > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 2/8/2019 12:17 AM, Chris Cunningham wrote: > On Wed, Feb 6, 2019 at 6:03 PM James Almer <jamrial@gmail.com> wrote: > >> Can a valid ogm stream contain more than one header packet? > > > Looking at ogg_packet oggdec.c, we read headers until hitting the first > non-header (counted in ogg stream field nb_headers), so multiple headers > per stream is probably ok. Probably ok as in our code currently allows it? Because if the spec disallows it, then that should be changed. The ogg demuxer currently looks for at most an specific amount of headers per stream. In general that means two, parameters and Vorbis comment metadata, and when the first non header packet is found it stops trying to read headers. What i mean with more than one header packet is for example two or more headers of a given type in the bitstream. I'm fairly sure only one of each is expected, so parsing any of them a second time, which is what in this sample of yours is resulting in codec_id NONE being propagated, is probably not correct. > But multiple codecs in a given stream seems > supsect to me. Someone with more ogg expertise should chime in. > > Because the >> issue here as far as i can see is that ogm_header() is not validating >> the output of ff_codec_get_id() and is happily accepting and propagating >> AV_CODEC_ID_NONE as derived from it in a late packet, long after the >> parser and everything else was already initialized. >> >> No other ogg module sets st->internal->need_context_update and all of >> them may also end up calling the respective header reading function more >> than once on invalid files, but unlike the ogm one, all of them hardcode >> the codec_id instead of blindly accepting a potentially bogus value >> derived from the bitstream. >> > > I'm open to hard-coding codec for gsm. No, ogm can have all kinds of codecs, hence it being defined in the bitstream. It should nonetheless have a check to make sure what's read is not AV_CODEC_ID_NONE. > One thing I notice is that the codec > is just one of several codecpar fields that are potentially changing. > If any of those change without need_context_update, it seems like values in > the internal avctx could become stale? That's why i was wondering if more than one header was allowed. I'm fairly sure it's not, and the demuxer should ignore any further header packet if it contains a header of a type already parsed.
2019-02-08 21:07 GMT+01:00, James Almer <jamrial@gmail.com>: > On 2/8/2019 12:17 AM, Chris Cunningham wrote: >> On Wed, Feb 6, 2019 at 6:03 PM James Almer <jamrial@gmail.com> wrote: >> >>> Can a valid ogm stream contain more than one header packet? >> >> >> Looking at ogg_packet oggdec.c, we read headers until hitting the first >> non-header (counted in ogg stream field nb_headers), so multiple headers >> per stream is probably ok. > > Probably ok as in our code currently allows it? Because if the spec > disallows it, then that should be changed. If our current code allows it, it should not be changed without a good reason and certainly not in a patch that fixes an abort. Carl Eugen
On 2/8/2019 5:51 PM, Carl Eugen Hoyos wrote: > 2019-02-08 21:07 GMT+01:00, James Almer <jamrial@gmail.com>: >> On 2/8/2019 12:17 AM, Chris Cunningham wrote: >>> On Wed, Feb 6, 2019 at 6:03 PM James Almer <jamrial@gmail.com> wrote: >>> >>>> Can a valid ogm stream contain more than one header packet? >>> >>> >>> Looking at ogg_packet oggdec.c, we read headers until hitting the first >>> non-header (counted in ogg stream field nb_headers), so multiple headers >>> per stream is probably ok. >> >> Probably ok as in our code currently allows it? Because if the spec >> disallows it, then that should be changed. > > If our current code allows it, it should not be changed without > a good reason and certainly not in a patch that fixes an abort. Chris' patch is making the demuxer reinitialize the context due to parameter changes taken from extra headers potentially found in the bitstream. In contrast, my suggestion is to instead make the demuxer skip said superfluous/duplicate headers if it turns out that extra parameter headers are not expected, which seems to be the case given that all the headers regardless of kind are expected to show up before the very first data packet. Both approaches will fix an assert in the parser as autoinserted by the generic libavformat code, but it's not strictly limited to that.
On Fri, Feb 08, 2019 at 05:07:03PM -0300, James Almer wrote: > On 2/8/2019 12:17 AM, Chris Cunningham wrote: > > On Wed, Feb 6, 2019 at 6:03 PM James Almer <jamrial@gmail.com> wrote: > > > >> Can a valid ogm stream contain more than one header packet? > > > > > > Looking at ogg_packet oggdec.c, we read headers until hitting the first > > non-header (counted in ogg stream field nb_headers), so multiple headers > > per stream is probably ok. > > Probably ok as in our code currently allows it? Because if the spec > disallows it, then that should be changed. > > The ogg demuxer currently looks for at most an specific amount of > headers per stream. In general that means two, parameters and Vorbis > comment metadata, and when the first non header packet is found it stops > trying to read headers. > > What i mean with more than one header packet is for example two or more > headers of a given type in the bitstream. I'm fairly sure only one of > each is expected, so parsing any of them a second time, which is what in > this sample of yours is resulting in codec_id NONE being propagated, is > probably not correct. > > > But multiple codecs in a given stream seems > > supsect to me. Someone with more ogg expertise should chime in. > > > > Because the > >> issue here as far as i can see is that ogm_header() is not validating > >> the output of ff_codec_get_id() and is happily accepting and propagating > >> AV_CODEC_ID_NONE as derived from it in a late packet, long after the > >> parser and everything else was already initialized. > >> > >> No other ogg module sets st->internal->need_context_update and all of > >> them may also end up calling the respective header reading function more > >> than once on invalid files, but unlike the ogm one, all of them hardcode > >> the codec_id instead of blindly accepting a potentially bogus value > >> derived from the bitstream. > >> > > > > I'm open to hard-coding codec for gsm. > > No, ogm can have all kinds of codecs, hence it being defined in the > bitstream. It should nonetheless have a check to make sure what's read > is not AV_CODEC_ID_NONE. > > > One thing I notice is that the codec > > is just one of several codecpar fields that are potentially changing. > > If any of those change without need_context_update, it seems like values in > > the internal avctx could become stale? > > That's why i was wondering if more than one header was allowed. I'm > fairly sure it's not, and the demuxer should ignore any further header > packet if it contains a header of a type already parsed. ogg allows chaining streams when they have differing serial numbers https://xiph.org/ogg/doc/oggstream.html i think ive seen actual files doing this ogg_replace_stream() might assign these into existing avstreams i think thx [...]
On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer <michaelni@gmx.at> wrote: > ogg allows chaining streams when they have differing serial numbers > https://xiph.org/ogg/doc/oggstream.html > > i think ive seen actual files doing this > > ogg_replace_stream() might assign these into existing avstreams i think > If I'm reading this correctly, I think that should always happen at the boundary of a new ogg page, meaning it wouldn't rely on this multiple headers logic? I found the commit where this was introduced https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split With the description: This fixes some old ogm files that had the 3rd vorbis header after a data packet in another stream. This is invalid in ogg, but this change shouldn't affect the behaviour of any valid file. So, I don't think we're going to find spec text for this. No spec for OGM and committer indicates its not valid Ogg. I'm guessing we want to still support these ogm files?
On Mon, Feb 11, 2019 at 1:55 PM Chris Cunningham <chcunningham@chromium.org> wrote: > On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer <michaelni@gmx.at> > wrote: > >> ogg allows chaining streams when they have differing serial numbers >> https://xiph.org/ogg/doc/oggstream.html >> >> i think ive seen actual files doing this >> >> ogg_replace_stream() might assign these into existing avstreams i think >> > > If I'm reading this correctly, I think that should always happen at the > boundary of a new ogg page, meaning it wouldn't rely on this multiple > headers logic? > > I found the commit where this was introduced > > https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split > > With the description: > This fixes some old ogm files that had the 3rd vorbis header after a data > packet in another stream. This is invalid in ogg, but this change shouldn't > affect the behaviour of any valid file. > > So, I don't think we're going to find spec text for this. No spec for OGM > and committer indicates its not valid Ogg. I'm guessing we want to still > support these ogm files? > > Hey friends, just checking in on this discussion. Pls advise on findings above. >
On Fri, Feb 15, 2019 at 02:56:18PM -0800, Chris Cunningham wrote: > On Mon, Feb 11, 2019 at 1:55 PM Chris Cunningham <chcunningham@chromium.org> > wrote: > > > On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer <michaelni@gmx.at> > > wrote: > > > >> ogg allows chaining streams when they have differing serial numbers > >> https://xiph.org/ogg/doc/oggstream.html > >> > >> i think ive seen actual files doing this > >> > >> ogg_replace_stream() might assign these into existing avstreams i think > >> > > > > If I'm reading this correctly, I think that should always happen at the > > boundary of a new ogg page, meaning it wouldn't rely on this multiple > > headers logic? > > > > I found the commit where this was introduced > > > > https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split > > > > With the description: > > This fixes some old ogm files that had the 3rd vorbis header after a data > > packet in another stream. This is invalid in ogg, but this change shouldn't > > affect the behaviour of any valid file. > > > > So, I don't think we're going to find spec text for this. No spec for OGM > > and committer indicates its not valid Ogg. I'm guessing we want to still > > support these ogm files? > > > > > Hey friends, just checking in on this discussion. Pls advise on findings > above. I have no real oppinion on which solution to pick. If disallowing these changes in OGM works, thats fine with me as is updating values thanks [...]
I'm fine to do either. James, do you still prefer to skip the later headers if this breaks some old ogm files? On Sat, Feb 16, 2019 at 4:52 PM Michael Niedermayer <michaelni@gmx.at> wrote: > On Fri, Feb 15, 2019 at 02:56:18PM -0800, Chris Cunningham wrote: > > On Mon, Feb 11, 2019 at 1:55 PM Chris Cunningham < > chcunningham@chromium.org> > > wrote: > > > > > On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer <michaelni@gmx.at> > > > wrote: > > > > > >> ogg allows chaining streams when they have differing serial numbers > > >> https://xiph.org/ogg/doc/oggstream.html > > >> > > >> i think ive seen actual files doing this > > >> > > >> ogg_replace_stream() might assign these into existing avstreams i > think > > >> > > > > > > If I'm reading this correctly, I think that should always happen at the > > > boundary of a new ogg page, meaning it wouldn't rely on this multiple > > > headers logic? > > > > > > I found the commit where this was introduced > > > > > > > https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split > > > > > > With the description: > > > This fixes some old ogm files that had the 3rd vorbis header after a > data > > > packet in another stream. This is invalid in ogg, but this change > shouldn't > > > affect the behaviour of any valid file. > > > > > > So, I don't think we're going to find spec text for this. No spec for > OGM > > > and committer indicates its not valid Ogg. I'm guessing we want to > still > > > support these ogm files? > > > > > > > > Hey friends, just checking in on this discussion. Pls advise on findings > > above. > > I have no real oppinion on which solution to pick. If disallowing these > changes in OGM works, thats fine with me as is updating values > > thanks > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The real ebay dictionary, page 2 > "100% positive feedback" - "All either got their money back or didnt > complain" > "Best seller ever, very honest" - "Seller refunded buyer after failed scam" > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham <chcunningham@chromium.org> wrote: > I'm fine to do either. James, do you still prefer to skip the later > headers if this breaks some old ogm files? > James, friendly ping
On 2/26/2019 10:18 PM, Chris Cunningham wrote: > On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham > <chcunningham@chromium.org <mailto:chcunningham@chromium.org>> wrote: > > I'm fine to do either. James, do you still prefer to skip the later > headers if this breaks some old ogm files? > > > James, friendly ping Yes, i'd prefer if we skip any superfluous parameter header that shows up before the first data packet. The return value of ff_codec_get_id() should also be checked to not propagate AV_CODEC_ID_NONE, which was the source of this issue.
On Thu, Feb 28, 2019 at 9:13 AM James Almer <jamrial@gmail.com> wrote: > On 2/26/2019 10:18 PM, Chris Cunningham wrote: > > On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham > > <chcunningham@chromium.org <mailto:chcunningham@chromium.org>> wrote: > > > > I'm fine to do either. James, do you still prefer to skip the later > > headers if this breaks some old ogm files? > > > > > > James, friendly ping > > Yes, i'd prefer if we skip any superfluous parameter header that shows > up before the first data packet. > The return value of ff_codec_get_id() should also be checked to not > propagate AV_CODEC_ID_NONE, which was the source of this issue. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel Renewing request to apply for this patch. The alternate route of identifying/skipping the bad packets was more difficult than expected (https://patchwork.ffmpeg.org/patch/13593/). James signed on this approach instead. Chris
Friendly ping. On Thu, Jun 20, 2019 at 11:17 AM Chris Cunningham <chcunningham@chromium.org> wrote: > On Thu, Feb 28, 2019 at 9:13 AM James Almer <jamrial@gmail.com> wrote: > >> On 2/26/2019 10:18 PM, Chris Cunningham wrote: >> > On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham >> > <chcunningham@chromium.org <mailto:chcunningham@chromium.org>> wrote: >> > >> > I'm fine to do either. James, do you still prefer to skip the later >> > headers if this breaks some old ogm files? >> > >> > >> > James, friendly ping >> >> Yes, i'd prefer if we skip any superfluous parameter header that shows >> up before the first data packet. >> The return value of ff_codec_get_id() should also be checked to not >> propagate AV_CODEC_ID_NONE, which was the source of this issue. >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > Renewing request to apply for this patch. > > The alternate route of identifying/skipping the bad packets was more > difficult than expected (https://patchwork.ffmpeg.org/patch/13593/). > James signed on this approach instead. > > Chris >
On 6/25/2019 1:44 PM, Chris Cunningham wrote: > Friendly ping. > > On Thu, Jun 20, 2019 at 11:17 AM Chris Cunningham <chcunningham@chromium.org> > wrote: > >> On Thu, Feb 28, 2019 at 9:13 AM James Almer <jamrial@gmail.com> wrote: >> >>> On 2/26/2019 10:18 PM, Chris Cunningham wrote: >>>> On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham >>>> <chcunningham@chromium.org <mailto:chcunningham@chromium.org>> wrote: >>>> >>>> I'm fine to do either. James, do you still prefer to skip the later >>>> headers if this breaks some old ogm files? >>>> >>>> >>>> James, friendly ping >>> >>> Yes, i'd prefer if we skip any superfluous parameter header that shows >>> up before the first data packet. >>> The return value of ff_codec_get_id() should also be checked to not >>> propagate AV_CODEC_ID_NONE, which was the source of this issue. >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >> Renewing request to apply for this patch. >> >> The alternate route of identifying/skipping the bad packets was more >> difficult than expected (https://patchwork.ffmpeg.org/patch/13593/). >> James signed on this approach instead. >> >> Chris Applied.
diff --git a/libavformat/oggparseogm.c b/libavformat/oggparseogm.c index a07453760b..b07a5d55ba 100644 --- a/libavformat/oggparseogm.c +++ b/libavformat/oggparseogm.c @@ -114,6 +114,9 @@ ogm_header(AVFormatContext *s, int idx) bytestream2_get_buffer(&p, st->codecpar->extradata, st->codecpar->extradata_size); } } + + // Update internal avctx with changes to codecpar above. + st->internal->need_context_update = 1; } else if (bytestream2_peek_byte(&p) == 3) { bytestream2_skip(&p, 7); if (bytestream2_get_bytes_left(&p) > 1)