[FFmpeg-devel] [PATCH] fix speex sample

[Baptiste Coudurier]

On 4/9/2009 2:14 PM, Michael Niedermayer wrote:
> On Thu, Apr 09, 2009 at 01:12:40PM -0700, Baptiste Coudurier wrote:
>> On 4/9/2009 12:37 PM, Michael Niedermayer wrote:
>>> On Thu, Apr 09, 2009 at 10:24:27AM -0700, Baptiste Coudurier wrote:
>>>> On 4/9/2009 7:06 AM, Michael Niedermayer wrote:
>>>>> On Thu, Apr 09, 2009 at 09:39:24AM +0200, Diego Biurrun wrote:
>>>>>> On Thu, Apr 09, 2009 at 04:46:34AM +0200, Michael Niedermayer wrote:
>>>>>>> On Wed, Apr 08, 2009 at 06:30:56PM -0700, Baptiste Coudurier wrote:
>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>> On Wed, Apr 08, 2009 at 05:26:11PM -0700, Baptiste Coudurier wrote:
>>>>>>>>>> On 4/8/2009 5:02 PM, Michael Niedermayer wrote:
>>>>>>>>>>> On Wed, Apr 08, 2009 at 04:21:32PM -0700, Baptiste Coudurier wrote:
>>>>>>>>>>>> Mpeg2 decoder has an important bug IMHO since some time. I
>>>>>>>>>>>> reported it, and libmpeg2 does not have this bug and decodes
>>>>>>>>>>>> correctly the first 2 frames. I lack some knowledge of the
>>>>>>>>>>>> surrounding code, but I tried to work on it at least. It should
>>>>>>>>>>>> not take you much time to figure out the problem I guess.
>>>>>>>>>>> thats a feature request not a bug, its something very well known
>>>>>>>>>>> since the code was written.
>>>>>>>>>> What was your argument about other implementation supporting it ? 
>>>>>>>>>> Oh yes, users will stop using yours to use the one supporting it.
>>>>>>>>>>
>>>>>>>>>> FYI, many of my samples use this mechanism, I just didn't really
>>>>>>>>>> realize it, I thought it was just broken link but finally, I
>>>>>>>>>> discovered that libavcodec deliberately _skip_ 2 frames, even
>>>>>>>>>> without telling you !
>>>>>>>>>>
>>>>>>>>>> Solution is simple, until fixed I will use libmpeg2.
>>>>>>>>> poor libmpeg2
>>>>>>>> I'd say poor libavcodec, loosing one heavy user because of lousy
>>>>>>>> maintainership.
>>>>>>> not implementing all feature requests != lousy maintainership
>>>>>>> if you want this feature, you can implement it.
>>>>>> So I will rephrase the question: What will it take for you to implement
>>>>>> this so that FFmpeg can be a complete libmpeg2 replacement?
>>>>> I honestly have no interrest in implementing this. This is not a feature
>>>>> that should affect any ones decission of which decoder to use.
>>>> I'm not sure but loosing these 2 frames is really annoying for me, I
>>>> basically loose frame accuracy.
>>> i dont see how, its a gamble if you will get 0, 1, 2 or even 99 frames prior
>>> of the keyframe if this feature request is implemented. And that is for
>>> each seek, not per file.
>> If you decode without seeking, you loose frame accuracy because these 2
>> frames are duplicated from the I frame following.
> 
> if closed_gop==0 that will still happen even when this feature request is
> implemented.

Well, in this case, at least, it is valid to refuse decoding them, but
not if closed_gop is set to 1.

>> That is the transcoded and the source file have different pictures at
>> the beginning of the stream.
> 
> encoders should not begin encoding with B frames, they begin with an I frame
> thus i cannot see how this could happen

Encoders _can_ according to specs, and I can understand why they do
this: your first gop structure is identical to following gops structure.

FFmpeg encoder outputs 10 frames for the first gop when specifying gop 12.

>> And if you don't duplicate frames you just loose 2 frames.
>>
>> You will seek based on pts. If you seek to 3 you will seek to the I
>> frame, if you seek to 1 you will seek to the I frame also since it is
>> before in coded order.
>>
> [...]
>> Basically:
>>
>> I pts 3
>> B pts 1
>> B PTS 2
> 
> if you seek to pts=1 in this case you will end up at the I frame of the
> previous GOP (which is not shown) or if such does not exist seeking will
> fail

It's not shown because it does not exist, this is the beginning of the
stream and closed_gop is set to 1.

Code should not seek to previous I frame is gop is closed in this case
anyway.

Does the current code really do this ? It seems anyway current PS
demuxers use dts.

> it is possible to fix this by using convergence duration based on the
> count of b frames, their duration and the closed_gop flag but i pitty
> the one who would try to implement this because it wont be fun.
> 
> 
>> Considering delay you almost have to double check the pts of the decoded
>> frame in any case IMHO.
>>
>>> how that improves accuracy relative to not receiving frames prior to
>>> the timestamp to which you seek, i dont understand.
>> If you don't duplicate frames, you get 2 frames less than the source file.
> 
> no, encoders should start with I frames not B frames

Yes, you get 2 frames less than the source file.

Encoders _can_ start with B frames according to specs. I agree they
shouldn't.

>>> [...]
>>>>> Also if someone considers this feature to be so critical to him, he
>>>>> can implement it, its simple "patch welcome"
>>>> Well, I'm not disagreeing here, I tried to fix it myself and submitted a
>>>> patch. I'm not sure to understand everything though.
>>>> Like I don't clearly understand why closed_gop and broken_link would
>>>> matter if we get error-concealment in this case.
>>>> Like you said these flags might be wrongly set.
>>> am i the only one that thinks that duplicating frames randomly through
>>> error concealment on every seek is a poor idea?
>>>
>>>
>>>> Do you mean that If broken_link is set, then these 2 frames cannot be
>>>> decoded and should be discarded ?
>>> no closed_gop must be set to 1 for the frames to be decodeable
>> Ok, what if it is wrongly set to 1 ? You must have concealment right ?
>> Isn't allocating last_picture_ptr enough ?
> 
> for functioning concealment? certainly not, it may be enough for avoiding
> a crash but it will not look pretty,
> best concealment strategy is likely to drop the frames if they access
> a non existent reference because they are not used as reference themselfs
> droping has very little vissible effect, a single wrong MV is very vissible
> (think of a green block in someones face)

Interesting, I don't get this: it is not ok to drop a partial packet but
it is ok to drop an entire frame which might be decoded correctly.
Anything obvious I miss ?

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org