[FFmpeg-devel] [PATCH] oggparseskeleton: return 0 on EOS packet

[James Almer]

On 06/01/14 7:22 PM, Reimar Döffinger wrote:
> 
> On 06.01.2014, at 22:07, James Almer <jamrial at gmail.com> wrote:
>> On 06/01/14 6:25 AM, Reimar Döffinger wrote:
>>> On 06.01.2014, at 08:25, James Almer <jamrial at gmail.com> wrote:
>>>> It's the last packet
>>>
>>> What does this fix?
>>> I'd rather not rely on EOS being set correctly unless there is an actual need.
>>> (not to mention that it's always helpful if the reason for the change is properly described in the log, so one can figure out what to test if it breaks something else and needs to be solved differently).
>>
>> I have a theora+skeleton file (with EOS set correctly) that spams a lot of "Broken file, 
>> keyframe not correctly marked." and a vp8+skeleton that spams "read_timestamp() failed in 
>> the middle" errors when using seek-test without this patch.
> 
> I'd like to really understand the why, this sounds like it might just hide the real bug/issue?
> 
>> With it the amount of warnings in the theora file goes down considerably, and the errors 
>> in the vp8 file disappear.
>> The output of seek-test also changes.
> 
> That would be more convincing if you said it is fixed, "changes" is not very convincing?

A quick test seeking with ffplay doesn't seem to show any easily discernible change, so i 
can't say it "fixed" anything beyond getting rid of a warning and error spam in seek-test 
with files that set skeleton EOS correctly.

>> That aside, returning 0 is correct since as i said the EOS packet is the last one.
> 
> I am fairly certain that one can trivially create a file where it isn't.

Which would be a broken file.

>> The behavior of broken files shouldn't change. It will still return -1 if the packet size 
>> is < 8, or 1 if none of the tests below the EOS one succeed (Which i think is wrong and 
>> should be changed to 0 or -1 since anything that's not "fishead", "fisbone" or EOS is 
>> invalid).
> 
> Maybe I misunderstood you, but I meant a file corruption where a packet in the middle ends up looking like a EOS one.

Without this patch, the demuxer never stops expecting header packets so any extra EOS 
would be handled by the parser same way as the first.
With it, however, if there's another packet after a valid 0-length EOS packet then it 
will not be handled by the skeleton parser (Unless we add a packet() function).
Not sure how the demuxer would handle it in such case, but probably the same way as 
it would with other stream types.

>> If you prefer i could make it so it returns -1 if the EOS packet size is > 0, which is the 
>> only other scenario that's not being considered.
> 
> Hm, maybe I'd have to first figure out what the different return values actually mean.
> Just intuitively I'd think that whether and where we have an EOS should impact behaviour only minimally - preferably not at all.

I uploaded both files to the ftp, theora_skel.ogv and vp8_skel.ogv if you want to check.