[FFmpeg-devel] [PATCH] HWAccel infrastructure

[Michael Niedermayer]

On Tue, Feb 17, 2009 at 08:14:55PM +0100, Gwenole Beauchesne wrote:
> Le 17 f?vr. 09 ? 19:46, Michael Niedermayer a ?crit :
[...]
> >> +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?

no objection from me ...


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

ok unless someone finds a better name


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

hmm, i thought of just adding a codec_id parameter to 
"av_find_hwaccel_codec_by_pix_fmt"


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


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


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

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090217/86dec00b/attachment.pgp>