[FFmpeg-devel] [PATCH][7/8] Add VA API accelerated H.264 decoding (take 4)

[Michael Niedermayer]

On Mon, Feb 09, 2009 at 10:25:49AM +0100, Gwenole Beauchesne wrote:
> Hi Michael,
> 
> On Fri, 6 Feb 2009, Michael Niedermayer wrote:
> 
> > Please factorize the code somehow.
> > I dont want to have all codecs salted with 2 dozen video acceleration APIs
> 
> This looks a little unfair to me.

i didnt claim to be fair


> I posted the VA API patches at the very 
> same time as VDPAU and the only complains at that time were indentation & 

i just checked ffmpeg-devel, but the first patch i can find from you is from
2009 01 06 while VDPAU was at least from 2008 11 16
i dont dispute that you did post them somewere but that seems not here ...


> cosmetics so I thought the approach was right. Now, you say the current 
> implementation is somewhat crap. I am sorry, it's pretty demotivating.

the issues ive raised apply to VDPAU & XVMC as well, this really isnt VAAPI
specifc, its rather issues that became clear as support for these hw accel
apis has been added ...
Trying to factorize code before more than 1 thing uses it is hard and can
lead to overabstracted academic nonsense.
So indeed whoever comes second has to do the factorization, ive applied
that "rule" to pretty much everything. And "comes second" is by what passes
review second it doesnt matter at all if thats by my incompetence/ignorance/
lazyness or someone elses.
This isnt fair but it is effective and practical, if you have a better
suggestion, its welcome of course, but not factorizing code or commiting
suboptimal code is not ok.

But you can also see it positive, if you factorize VDPAU + VAAPI support
while nvidia doesnt help (and i dont belive they wont ...)
you can maybe gain some advantage for VAAPI over VDPAU ;)


[...]
> I'd pretty would like VA API support integrated for the next FFmpeg 
> release

everyone wants their patch integrated for the next release


[...]

> > B. a AVHWAccel struct is added similar to AVCodec and that codecs are
> > only allowed to call its members like start_frame() start_slice() end_frame()
> > and such
> 
> Where would that struct be initialized? Having each codec calling an 
> ff_API_CODEC_decode_init() function or have avcodec_get_context_defaults() 
> call an ff_API_get_defaults() to initialize the vtable? Though, 
> avctx->codec wouldn't be set at this time.
> 
> Suggested struct:
> 
> typedef struct AVHWAccel {
>      void *avctx;
>      unsigned int flags; ///< Hardware decode pipeline configuration flags
>      int (*start_frame)(AVCodecContext *avctx);
>      int (*start_slice)(AVCodecContext *avctx, const uint8_t *buffer, uint32_t size);
>      int (*decode_slice)(AVCodecContext *avctx);
>      int (*end_slice)(AVCodecContext *avctx);
>      int (*end_frame)(AVCodecContext *avctx);
> } AVHWAccel;

ehm this is close but not exactly what i had in mind
i more had in mind:

typedef struct AVHWAccel {
    /** (copied from AVCodec)
     * Name of the codec implementation.
     * The name is globally unique among encoders and among decoders (but an
     * encoder and a decoder can share the same name).
     * This is the primary way to find a codec from the user perspective.
     */
    const char *name;
    enum CodecType type;
    enum CodecID id;
    int (*start_frame)(AVCodecContext *avctx);
    int (*start_slice)(AVCodecContext *avctx, const uint8_t *buffer, unsigned size);
    int (*decode_slice)(AVCodecContext *avctx);
    int (*end_slice)(AVCodecContext *avctx);
    int (*end_frame)(AVCodecContext *avctx);
    int capabilities;
    const enum PixelFormat *pix_fmts;       ///< array of supported pixel formats, or NULL if unknown, array is terminated by -1
    /** (copied from AVCodec)
     * Descriptive name for the codec, meant to be more human readable than \p name.
     * You \e should use the NULL_IF_CONFIG_SMALL() macro to define it.
     */
    const char *long_name;

    struct AVHWAccel *next;
} AVHWAccel;

av_register_hw_accel(struct AVHWAccel *hw_accel);

and adding a AVHWAccel *hw_accel field to AVCodecContext
and adding to codecs
if(ctx->hw_accel)
    ctx->hw_accel->start_frame(ctx);

anyway above is just a rough draft ...


> 
> But there is a problem with this approach. Sometimes, the size of the 
> slice can't be known beforehand. e.g. MPEG-4. Unless you accept to 
> duplicate code to move to the next resync marker or end of buffer, I don't 
> see how to elegantly handle this code.
> 

> Probably make start_slice() take a buffer base + offset into this buffer 
> (for internal optimisation => use a single whole buffer to emit) and have 
> end_slice() take the buffer_end (or size wrt. the previous call to 
> start_slice())?

possible
its you who knows better what integrates best with the V* APIs


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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20090209/5a5fa1b5/attachment.pgp>