[FFmpeg-devel] [PATCH] HWAccel infrastructure

[Gwenole Beauchesne]

Le 17 f?vr. 09 ? 19:46, Michael Niedermayer a ?crit :

>> /**
>> + * AVHWAccelCodec.
>> + */
>> +typedef struct AVHWAccelCodec {
>
> I would prefer it to be named AVHWAccel

Why not but I wanted to avoid to be hit by other consistency lords for  
almost duplicating AVCodec semantics without an *Codec name or  
whatever...

>> +int av_hwaccel_decode_slice(AVCodecContext *avctx, const uint8_t  
>> *buf, uint32_t buf_offset, uint32_t buf_size);
>
> i suspect this is not supposed to be called by the user, thus
> it does not belong in a public header.
> Also private API is ff_ public is av_

So, can I create an avhwaccel.h header and then rename all  
{xvmc,vdpau,vaapi}.c to hwaccel_*.c for consistency? Or, is there any  
other suitable file that already exists?

>> +AVHWAccelCodec *av_find_hwaccel_codec(AVCodecContext *avctx,  
>> AVCodec *codec);
>
> this surely is a poor name for the function, its really querring the  
> user app

ff_query_hwaccel_codec()?
ff_find_user_hwaccel_for_codec()?

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

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

>> +                    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? I can drop that buf_ptr = next_slice; it will be done on the next  
iteration but we would have looked for the next start_code twice.

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

>> +        if (c->type == codec->type && c->id == codec->id)
>> +            pix_fmts[n_pix_fmts++] = c->pix_fmt;
>> +    }
>> +    if (n_pix_fmts == 0)
>> +        return NULL;
>> +    pix_fmts[n_pix_fmts] = PIX_FMT_NONE;
>> +
>
>> +    if ((pix_fmt = avctx->get_format(avctx, pix_fmts)) !=  
>> PIX_FMT_NONE)
>> +        return av_find_hwaccel_codec_by_pix_fmt(pix_fmt);
>
> i dont see why a PIX_FMT_NONE check is needed here

Previous implementation left-over. ;-) And it will be nuked away  
anyway if you approve the two-array temporaries.

Thanks,
Gwenole.