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

2019-06-25 Thread James Almer
On 6/25/2019 1:44 PM, Chris Cunningham wrote:
> Friendly ping.
> 
> On Thu, Jun 20, 2019 at 11:17 AM Chris Cunningham 
> wrote:
> 
>> On Thu, Feb 28, 2019 at 9:13 AM James Almer  wrote:
>>
>>> On 2/26/2019 10:18 PM, Chris Cunningham wrote:
 On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham
 mailto:chcunning...@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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2019-06-25 Thread Chris Cunningham
Friendly ping.

On Thu, Jun 20, 2019 at 11:17 AM Chris Cunningham 
wrote:

> On Thu, Feb 28, 2019 at 9:13 AM James Almer  wrote:
>
>> On 2/26/2019 10:18 PM, Chris Cunningham wrote:
>> > On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham
>> > mailto:chcunning...@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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2019-06-20 Thread Chris Cunningham
On Thu, Feb 28, 2019 at 9:13 AM James Almer  wrote:

> On 2/26/2019 10:18 PM, Chris Cunningham wrote:
> > On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham
> > mailto:chcunning...@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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2019-02-28 Thread James Almer
On 2/26/2019 10:18 PM, Chris Cunningham wrote:
> On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham
> mailto:chcunning...@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


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

2019-02-26 Thread Chris Cunningham
On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham 
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-21 Thread Chris Cunningham
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 
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 <
> chcunning...@chromium.org>
> > wrote:
> >
> > > On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer 
> > > 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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-16 Thread Michael Niedermayer
On Fri, Feb 15, 2019 at 02:56:18PM -0800, Chris Cunningham wrote:
> On Mon, Feb 11, 2019 at 1:55 PM Chris Cunningham 
> wrote:
> 
> > On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer 
> > 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"


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-15 Thread Chris Cunningham
On Mon, Feb 11, 2019 at 1:55 PM Chris Cunningham 
wrote:

> On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer 
> 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.

>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-11 Thread Chris Cunningham
On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer  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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-08 Thread Michael Niedermayer
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  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

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The smallest minority on earth is the individual. Those who deny 
individual rights cannot claim to be defenders of minorities. - Ayn Rand


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-08 Thread James Almer
On 2/8/2019 5:51 PM, Carl Eugen Hoyos wrote:
> 2019-02-08 21:07 GMT+01:00, James Almer :
>> On 2/8/2019 12:17 AM, Chris Cunningham wrote:
>>> On Wed, Feb 6, 2019 at 6:03 PM James Almer  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-08 Thread Carl Eugen Hoyos
2019-02-08 21:07 GMT+01:00, James Almer :
> On 2/8/2019 12:17 AM, Chris Cunningham wrote:
>> On Wed, Feb 6, 2019 at 6:03 PM James Almer  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-08 Thread James Almer
On 2/8/2019 12:17 AM, Chris Cunningham wrote:
> On Wed, Feb 6, 2019 at 6:03 PM James Almer  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-08 Thread Chris Cunningham
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(, 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() == 3) {
 bytestream2_skip(, 7);
 if (bytestream2_get_bytes_left() > 1)
-- 
2.20.1.791.gb4d0f1c61a-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-08 Thread Chris Cunningham
I'll re-send with my full name.

On Fri, Feb 8, 2019 at 4:57 AM Michael Niedermayer 
wrote:

> Hi
>
> On Wed, Feb 06, 2019 at 05:13:03PM -0800, chcunningham wrote:
> > From: chcunningham 
>
> 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)
>  Could you stop committing things like this?
>  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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-08 Thread Michael Niedermayer
Hi

On Wed, Feb 06, 2019 at 05:13:03PM -0800, chcunningham wrote:
> From: chcunningham 

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)
 Could you stop committing things like this?
 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 ..."


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-07 Thread Chris Cunningham
On Wed, Feb 6, 2019 at 6:03 PM James Almer  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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-06 Thread James Almer
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(, 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() == 3) {
>  bytestream2_skip(, 7);
>  if (bytestream2_get_bytes_left() > 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-06 Thread Chris Cunningham
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 
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(, 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() == 3) {
>  bytestream2_skip(, 7);
>  if (bytestream2_get_bytes_left() > 1)
> --
> 2.20.1.611.gfbb209baf1-goog
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2019-02-06 Thread chcunningham
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(, 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() == 3) {
 bytestream2_skip(, 7);
 if (bytestream2_get_bytes_left() > 1)
-- 
2.20.1.611.gfbb209baf1-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel