[FFmpeg-devel] [PATCH] correct MPEG video parser not to return incomplete frames

[Ivan Schreter]

Hi,

Michael Niedermayer wrote:
> [...]
>>> AVStream.initial_bytes_to_discard
>>> and that amount is discarded after the parser
>>>   
>>>       
>> A possiblity, yes. But who will fill it and when?
>>     
>
> your seeking code, and when a seek happens
>
>   

Easier said than done... For that, I suppose the parser needs to be 
extended to deliver this information. And it can only deliver it, if it 
knows the partial frame at the beginning of the packet is not a frame, 
so it's the same problem.

>   
>> I actually find your suggestion of fixing the misassociation of the 
>> timestamp to the frame first (at the end) much better. So let's concentrate 
>> on that one.
>>     
>
> You speak of them as if they where 2 alternatives, they are not!
>   

Basically, they are, in the sense how to seek in the file properly. 
Either the parser is able to detect invalid data at beginning of the 
frame and tell us this, or the parser will associate the timestamp with 
the frame starting in the packet (and not erroneously with the previous 
one, which in our case happens to be the partial non-frame).

Of course, doing both makes sense, since we can skip the non-frame at 
the beginning, not feeding the non-frame to the decoder later.

> ... neither where the 4 h264 timestamp cases from the spec, you implemented
> one leaving 3/4 of the cases unsupported ...
>   

???

And what's wrong with that? I'm trying to contribute to the project to 
address some use cases. I implemented the handling for streams which 
define cpb_removal_delay and dpb_output_delay. For some of the other 
cases, there is already some generic implementation, which (sort of) 
works. If someone needs to support timestamps in streams which don't 
specify the above, he's free to submit a patch or feature request so 
somebody else can implement it.

> here we do have a bug with timestamps being associated wrongly, this should be
> fixed
>   

Agreed. I'm looking into that.

> Also the mpeg demuxer returns random trash prior to the first frame, this IS
> a bug in your seeking code. Fix it please! And before you start arguing about
> how unlikely it would cause a problem in mpeg video, it will cause problems
> in mpeg audio as well.
>   

I don't understand what random trash is returned from MPEG demuxer prior 
to first frame. Also, I don't understand why the seeking code should be 
causing this, since it positions the file to start reading at the packet 
with start of a frame. Old seeking code didn't do it differently...

How can I reproduce the problem? With which sample file?

> The goal of the seeking code is to be exact, if now the first frame can be
> invalid without this being detectable the decoder or parser would have to
> discard it and this would break exactness
>   

I agree with you. But again, the parser has to somehow detect the broken 
non-frame. And you disagree with detection of non-frame by detecting 
missing picture start code and slice start code (concretely, for MPEG 
video). So how am I supposed to detect such non-frame, so I can 
initialize AVStream.initial_bytes_to_discard?

>
> [...]
>   
>>> [...]
>>> IIRC our parser misassociates a timestamp in the partial frame case, this
>>> has to be fixed _first_. I am against adding new features on top of known
>>> bugs.
>>>   
>>>       
>> I looked at the code in mpegvideo_parse() and ff_mpeg1_find_frame_end(). To 
>> me, it seems like ff_mpeg1_find_frame_end() misassociates the timestamp. 
>> Namely, the timestamp is fetched there, but actually the _previous_ frame 
>> (or in this case, a non-frame) is returned from the parsing routine.
>>
>> I'm not sure how to fix this, though. Possibly, instead of fetching the 
>> timestamp in ff_mpeg1_find_frame_end(), it should be done in this way:
>>
>>     
>
>   
>>    * If the frame starts at offset 0 in the PES packet AND we don't
>>      have anything in the buffer, then fetch the timestamp.
>>     
>
> sounds like another one of your not always working hacks
>   

Grrrr....

Man, I'm trying to help! Instead of bashing, make a proposal how to do it!

I'm again at the point of simply giving up and investing my limited time 
into something else, where people value the contribution a bit more...

Regards,

Ivan