[FFmpeg-devel] [PATCH] HWAccel infrastructure (take 3)

[Gwenole Beauchesne]

On Wed, 18 Feb 2009, Michael Niedermayer wrote:

>>>> Actually, the correct sequence order was meant to be start_slice(),
>>>> decode_slice(), end_slice() so that to follow natural processing of the
>>>> bitstream in the codec implementation. Otherwise, we can end up
>>>> duplicating code to advance to the next slice and/or add other variables
>>>> to record the previous position then compute size. i.e. IMO, uglification
>>>> in some locations (in h263dec.c in particular).
>>>>
>>>> If it's OK with you to slightly uglify h263dec but make it possible to
>>>> merge all 3 into a single decode_slice() call, then that'd be fine with
>>>> me
>>>> too.
>>>
>>> start&decode as in your patch can be merged without any other changes

OK, done locally.

>>> i cant speak about if i would accept some uglification without really
>>> seeing
>>> it
>>> also considering the special casing in mpeg1/2 it does not seem the split
>>> in
>>> 3 really avoids uglification
>>
>> OK, I probably could do something like the following in
>> h263dec.c::decode_slice():
>>
[... ugly code snippet ...]
>> That should be enough. Would that be OK with you (stylistically speaking)?
>
> no

So, do you finally like my start_slice()/end_slice() approach with 
buf+buf_end? ;-) The way h263dec.c is currently written is my only reason 
I went for this approach. I don't see other elegant and minimalistic 
solution for that.

>>>> Concerning buf / buf_offset, this was meant for optimization purposes in
>>>> the HWAccel implementation. That way we can reduce control blocks and/or
>>>> (it depends on the HWAccel) copy only one buffer in the end.
>>>
>>> if you have buf + buf_size and the buffers overlap you can do that too,
>>> besides if you have a buf/buf_size from start_frame you could check if
>>> the slices buffer is within that.
>>
>> It depends, if the hardware accelerator wants the unescaped data (e.g. for
>> H.264 and VC-1), then the check wouldn't be valid since the passed args
>> would be from another (temporary) buffer. On the other hand, VA API and
>> VDPAU are working fine with the escaped (raw bitstream) data.
>
> with a temporary buffer buf_offset/buf can be anything
> equal, larger or smaller ...
> the check against the start_frame buf/buf_size though should still work

OK, we can leave the "HW accelerator wants the unescaped data" case to 
someone else.

>> buf/buf_size from start_frame would be the buf/buf_size from
>> AVCodec::decode_frame()?
>
> that was my idea

Is there an HW accelerator able to grok that directly? But anyway, I am 
not against it, it's just that I see issue considering the things below.

>> I really want start_frame() to be at a location
>> where we are assured that frame info is parsed.
>
> should not be a problem ...

It is for e.g. h264.c, reasons kept below.

>> For h264.c, that'd be a little problematic. I could use the same mpeg12.c
>> trick to use s->first_slice and:
>>
>> NAL_SLICE: [...]
>>             if((err = decode_slice_header(hx, h)))
>>                break;
>>             if(s->avctx->hwaccel && s->first_slice){
>>                 s->first_slice=0;
>>                 s->avctx->hwaccel->start_frame(s->avctx, buf, buf_size);
>>             }
>>
>> but I might not get the whole frame buffer. Or I could place the
>> start_frame() call where ff_vdpau_h264_set_reference_frames(s); currently
>> is. But in that case, I might have the following AVHWAccel hooks calling
>> sequence: [?start_slice() ]+, end_slice(), start_frame(), end_frame(),
>> which is not really desirable.
>>
>

>> IMO, this complicates a little the code for no real gain. I don't actually
>> need buf/buf_size in neither VA API nor VDPAU.
>
> what happens with a buf of random content and buf_size of 1
> will it segfault? if not how do you know how much you can copy? did i miss
> some buf_size parameter somewhere?

FFmpeg always sends correct data to users, it won't segfault and it's not 
lavc's fault if users can't do a simple subtraction. ;-)

so we have start_slice() with a buf+offset and end_slice() with a buf_end, 
so we can reconstruct the buf_size.