[FFmpeg-cvslog] r19091 - trunk/libavcodec/4xm.c

[Baptiste Coudurier]

Hi Michael,

Michael Niedermayer wrote:
> On Sat, Jun 06, 2009 at 01:49:35AM -0700, Baptiste Coudurier wrote:
>> Baptiste Coudurier wrote:
>>> On 6/5/2009 2:25 PM, Michael Niedermayer wrote:
>>>> On Fri, Jun 05, 2009 at 02:09:05PM -0700, Baptiste Coudurier wrote:
>>>>> On 6/5/2009 1:10 PM, Michael Niedermayer wrote:
>>>>>> On Fri, Jun 05, 2009 at 10:12:14AM +0200, bcoudurier wrote:
>>>>>>> Author: bcoudurier
>>>>>>> Date: Fri Jun  5 10:12:14 2009
>>>>>>> New Revision: 19091
>>>>>>>
>>>>>>> Log:
>>>>>>> 4xm decoder uses get_buffer, set CODEC_CAP_DR1
>>>>>> i think you broke several decoders with these changes, no i dont
>>>>>> know which but
>>>>>> CODEC_CAP_DR1 does not just mean the codec uses get_buffer()
>>>>>> but it means it works with any get_buffer() implementation while
>>>>>> lack of CODEC_CAP_DR1 means it requires the libavcodec internal get_buffer()
>>>>> Interesting, I did misunderstand the Doxygen then:
>>>>> /**
>>>>>  * Codec uses get_buffer() for allocating buffers.
>>>>>  * direct rendering method 1
>>>>>  */
>>>>> #define CODEC_CAP_DR1             0x0002
>>>>>
>>>>>> one case (that applies to 4xm IIRC) is that linesize == width*bpp is required
>>>>>> by some decoders
>>>>> Well, user can override the function in any case, so decoder must not
>>>>> use get_buffer in AVCodecContext IMHO.
>>>> hmm, the original idea i think was that the user would not override
>>>> get_buffer when CODEC_CAP_DR1 is not set but it seems not documented
>>>> anywhere, this is of course not good
>>>>
>>>> we either could fix the docs and call everyone who overrides get_buffer
>>>> without a CODEC_CAP_DR1 buggy
>>>> or
>>>> change decoders to not use (re)get_buffer/release_buffer but the default
>>>> code directly
>>>>
>>>> IMHO, because overriding get_buffer without a set CODEC_CAP_DR1 always was
>>>> buggy i think user apps should be fixed that do this
>>>> I mean even if we change codecs to use default_*buffer() directly, user apps
>>>> would still fail to work with old libavcodec
>>>> thus IMHO this is a bug in the docs not the code
>>> IMHO, codecs should be updated to use avcodec_default_get_buffer
>>> directly which will avoid confusion.
>> Well, I need to revise my statement. Doing that for old codecs this
>> might have weird effects for users already overriding get_buffer,
>> example the old way of setting pts used by ffplay, I think many
>> applications did that unfortunately and ffplay didn't check for
>> CODEC_CAP_DR1 IIRC.
> 
> I really think updating the docs is the most feaseable solution, we
> did in the past even litterally recommand to use get_buffer() for
> pts ...
> 
> I would suggest to add a simple
> "if CODEC_CAP_DR1 is not set then (re)get_buffer() must call
>  avcodec_default_get_buffer() instead of providing buffers allocated by
>  some other means"
> to the (re)get_buffer doxy

Ok.

>>> [...]
>>>
>>> I'll check decoders and revert/modify them on a per case basis once we
>>> settled this.
>>>
>> Btw, If I understood correctly how this works, here is a patch for 4xm
>> decoder which should use reget_buffer.
> 
> please elaborate why you think reget_buffer() would be needed

4xm can contain p-frames, when decoder requests a buffer for p, it seems
to need the last picture painted into p because it will only write the diff.

It seems to work with get_buffer currently because internal get_buffer
will not change buffers if w/h/stride does not change, and returns same
allocated buf->base (release will do internal_count-- just before).

Maybe it's more complicated than I think :>

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org