[FFmpeg-devel] [PATCH] HWAccel infrastructure

[Gwenole Beauchesne]

Hi,

Le 17 f?vr. 09 ? 20:33, Michael Niedermayer a ?crit :

>>>> +AVHWAccelCodec *av_find_hwaccel_codec_by_pix_fmt(enum PixelFormat
>>>> pix_fmt);
>>>
>>> pix_fmts do not neccesarily implicate the codec_id, its just what
>>> current hw
>>> accels do. Besides its not logic if the pix_fmt implictes the  
>>> codec_id
>>
>> Is it then OK to create two arrays in the ff_query_hwaccel_codec()
>> function? One to know the pix_fmt, and the other one to store the
>> matching AVHWAccel. It turns out av_find_hwaccel_codec_by_pix_fmt()  
>> is
>> not used outside utils.c anyway. Then, we will even be able to return
>> the hwaccel right away without traversing the map table again.
>
> hmm, i thought of just adding a codec_id parameter to
> "av_find_hwaccel_codec_by_pix_fmt"

Yes, but what if there were to be audio accelerators too? It looked  
simpler to me to just pass an AVCodec (to the other function). Anyway,  
as I had mentioned afterwards, this function will be dead. It should  
be clearer in a few hours once I produce a patch at the office.

As a side note, on my Atom + Poulsbo platform, it became apparent that  
the second most CPU demanding task for decoding was audio (by more  
than 30%) once the video was offloaded to the VPU. It doesn't mean  
there does exist means to offload audio decoding, just that there  
might be in the future, though I pretty doubt.

(on the other hand, is there any codec standard that specifies both  
audio and video at the same time? if not, a codec_id is indeed enough).

>>>> +                if (avctx->hwaccel_codec) {
>>>> +                    const uint8_t *curr_slice = buf_ptr - 4;
>>>> +                    const uint8_t *next_slice;
>>>> +                    start_code = -1;
>>>
>>>> +                    next_slice = ff_find_start_code(buf_ptr + 2,
>>>> buf_end, &start_code);
>>>
>>> are you doing another pass over the bitstream? if so this is
>>> unacceptable
>>
>> I am just trying to reuse existing code to know where the current
>> slice ends. How could I achieve this purpose otherwise?
>
> how does vdpau do it? a quick look shows no ff_find_start_code() but
> i may have missed it

VDPAU can offload the whole picture data. VA API specifies this as a  
per-slice process, i.e. emit all individual slice, one control block  
at a time (VASliceParameterBufferMPEG2). However, in practise and  
IIRC, the current Intel implementation does support the once control  
block at a time approach (akin to NVIDIA's VDPAU implementation) but  
it's just not specified to work that way. So, I need a way to advance  
to the next slice, thus determining the actual size of the slice.

>>>> +                    if (next_slice < buf_end)
>>>> +                        next_slice -= 4;
>>>> +                    s2->resync_mb_x= s2->mb_x;
>>>> +                    s2->resync_mb_y= s2->mb_y= mb_y;
>>>
>>>> +                    if (av_hwaccel_decode_slice(avctx, buf,
>>>> curr_slice - buf, next_slice - curr_slice) < 0)
>>>> +                        return -1;
>>>> +                     buf_ptr = next_slice; /* don't traverse the
>>>> whole slice again in ff_find_start_code() */
>>>> +                     break;
>>>
>>> your indention is not looking good ...
>>
>> Why?
>
> because some lines have n and some n+1 spaces before them

OK, I had read your "inTention is not looking good", in reference to  
the comment near. ;-)

>>>> +    assert(codec->start_frame);
>>>> +    assert(codec->end_frame);
>>>> +    if (codec->decode_slice) {
>>>> +        assert(codec->start_slice);
>>>> +        assert(codec->end_slice);
>>>> +    }
>>>
>>> no, assert cannot be used to check user provided parameters
>>> either dont check or check properly with error messages and error
>>> return but i suspect checking is overkill for this
>>
>> Well, this would be "one-short" error from an implementor point of
>> view. It's only to help him make sure it implemented all the  
>> necessary
>> funcs. On the other hand, should we care beyond what will be
>> documented? If not, I can happily remove the assert()s and then crash
>> at runtime and have him RTFM.
>
> the thing is assert(0) will not do anything if NDEBUG is set during
> build, that makes it kinda useless for anything outside libav*
> development

Exactly, it was only meant to be useful during libav* development. It  
does not really matter otherwise, the developer would already have  
tested his code at least once prior to releasing his code. ;-)

Regards,
Gwenole