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

[Michael Niedermayer]

On Sat, Sep 05, 2009 at 10:20:22PM +0200, Ivan Schreter wrote:
> Hi,
>
> Michael Niedermayer wrote:
[...]
>> ... 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.

there is nothig wrong with it as such, its just annoying to have things
implemented halfway. Of course half is better than nothing but its not as
good as a full implementation of the stuff ...


>
>> 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?

any mpeg-ps file created with ffmpeg
and to make it more understandable look at:
"produce the problem"

it looks like a correct phrase, but it could really have been "reproduce ..."
if you dont know the previous chars ...

you seem to belive that you can cut an ES stream at any byte position and the
decoder can even in theory pick up decoding at the first complete frame while
discarding previous frames.
for mpeg video that does work in general though not in all corner cases.
for mpeg audio it does not work, there is nothing preventing the startcode
from occuring inside a frame (unless i am too tired and silly but i dont
think so ...)
To make it even more clear what the problem is, look at this example, 
the seeking code seeks to packet 5, with the expectation that the decoder
will decode the first frame in there but there is a startcode emulation
in the previous partial frame causing the decoder not only to return noise
but also to miss the first complete frame. The only way for the decoder to
avoid this is to drop the first few frames but if it does, it starts decoding
later than when the seeking code expects.
Its the seeking code that should start a little earlier and discard data
to make sure synchronization of frame boundaries did happen
its what i suggested already previously for fixing the video corner cases


>
>> 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?

you start a frame or 2 earlier and throw them away, with each the probability
of being off drops


>
>>
>> [...]
>>   
>>>> [...]
>>>> 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!

iam not sure how to implement all the stuff down to the finest detail without
new issues poping up, but iam sure you take this too serious ...

the first partial frame has no pic startcode, it cant otherwise it would be
the correct target for the timestamp so  why is our parser associateing 
the timestamo with it? ive not had time to look at the actual code more
carefully sadly ...
The timestamp association rules are quite clear in mpeg, the parser really
should not need any black magic to get it right even if it doesnt know if the
first part is a complete frame or not.


>
> 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...

your contribution is valued but implementing this is not a trivial task and
writing a functioning implementation does take time, you seem to expect that
a quick implementation would fully work

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090906/53532b13/attachment.pgp>