[FFmpeg-devel] avformat/oggparseogm: sync avctx w/ codecpar

Submitted by chcunningham@chromium.org on Feb. 7, 2019, 1:13 a.m.

Details

Message ID 20190207011303.226684-1-chcunningham@chromium.org
State New
Headers show

Commit Message

chcunningham@chromium.org Feb. 7, 2019, 1:13 a.m.
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(+)

Comments

chcunningham@chromium.org Feb. 7, 2019, 1:16 a.m.
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
>
James Almer Feb. 7, 2019, 1:57 a.m.
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.
chcunningham@chromium.org Feb. 8, 2019, 3:17 a.m.
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?
Michael Niedermayer Feb. 8, 2019, 12:57 p.m.
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"

[...]
chcunningham@chromium.org Feb. 8, 2019, 7:45 p.m.
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
>
James Almer Feb. 8, 2019, 8:07 p.m.
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.
Carl Eugen Hoyos Feb. 8, 2019, 8:51 p.m.
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
James Almer Feb. 8, 2019, 9:21 p.m.
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.
Michael Niedermayer Feb. 8, 2019, 10:37 p.m.
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

[...]
chcunningham@chromium.org Feb. 11, 2019, 9:55 p.m.
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?
chcunningham@chromium.org Feb. 15, 2019, 10:56 p.m.
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.

>
Michael Niedermayer Feb. 17, 2019, 12:52 a.m.
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

[...]
chcunningham@chromium.org Feb. 22, 2019, 12:46 a.m.
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
>

Patch hide | download patch | download mbox

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)