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

[James Almer]

On 2/8/2019 12:17 AM, Chris Cunningham wrote:
> On Wed, Feb 6, 2019 at 6:03 PM James Almer <jamrial at 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.