[FFmpeg-devel] [PATCH][VAAPI][6/6] Add H.264 bitstream decoding (take 17) (ping)

[Michael Niedermayer]

On Mon, Jun 08, 2009 at 02:24:27PM +0200, Cyril Russo wrote:
> Michael Niedermayer a ?crit :
>> On Thu, Jun 04, 2009 at 02:57:54PM +0200, Cyril Russo wrote:
>>   
>>> Michael Niedermayer a ?crit :
>>>     
>>>>         
>>>>> +static void fill_reference_pic_list(VAPictureH264 RefPicList[32],      
>>>>>        
>>>> [...
>>>>         
>>>>> +static void h264_fill_plain_pred_weight_table(H264Context *h, int 
>>>>> list,
>>>>>             
>>>> [...]
>>>>         
>>>>> +
>>>>> +/** Initialize and start decoding a frame with VAAPI. */
>>>>> +static int vaapi_h264_start_frame(AVCodecContext *          avctx,     
>>>>>         
>>>> i must say the prefixes still are entirely random
>>>> vaapi_h264_ here h264_ above and nothing above that
>>>> it all would be fine where there a difference but its all h264 and vaapi
>>>> related code.
>>>>         
>>> Ok, here's the idea I've understood from other files, let me know if I'm 
>>> wrong.
>>> - vaapi_codecname_[...] are the exported functions (ex: 
>>> vaapi_vc1_start_frame, vaapi_h263_start_frame, vaapi_h264_start_frame...)
>>>     
>>
>> they are not really exported, they are static, pointers to them are
>> exported
>>   
>
> Sure, I know this. But, as I don't want to modify the other vaapi files
> (at least, not in that patch), I think it's better to be consistent with
> the other code.
> In the other files, the names are build following the pattern
> vaapi_##CODEC##_##function
>
> There are 3 of them in each files, by the end of the file, so they can
> be located quite easily.
> Unless you expressely say to rename them, I'd prefer to keep them that way.

rename the ones in this patch, the other files can be dealt with at some
point in the future ...


>
> I've completed the initial comment to state so.
>
>>
>>> +/** Fill VAAPI's reference frames array. */
>>>     
>> [...]
>>
>>   
>>> +/** Fill VAAPI reference picture lists.
>>>     
>>
>> what is the difference between the 2 ?
>>
>>
>>   
> The picture isn't the frame (...you know this..., so I guess the real
> question was "Why is there a difference between the 2?")

no, really i think the doxy assumes that a reader is familiar with the
definitions of frame and picture (from MPEG) as well as VAAPI and H.264
in other words i think it could be a little more verbose


>
> Generally speaking, the reference frame for VAAPI is linked to a
> hardware buffer by an index.
> As specified elsewhere in the file, VAAPI doesn't infer any value, so it
> must have access to all the data for its operations (even defaults)
>
> The former takes a FFmpeg's H264 context, and for each picture with a
> (nonzero) reference, create a reference frame entry in the VAAPI ref's
> array.
> It is only used in the start_frame function once.

the function uses variables that differ between slices, i doubt that that
works except by luck that they tend not to differ in practice.


> It's not an initialization function for the given argument, as it only
> deal with the reference frame array of the argument.
> It's used to keep track of frames that the decoding process need to
> refer too later on, and more specifically, when using interlaced content
> (as VAAPI require merged top and bottom field IIUC).
>
>
> The latter is called in the decode_slice function. It minimally fills
> the reference picture list for the current slice (as most of the work
> was done in the former),
> and only for the actual reference pictures (of the slice).
> It acts on another list of reference than the former, as it deals with
> VAAPI's slice reference list.
>
> So I guess the process is more like:
> 1) When starting a frame, prepare the reference frames, and if
> interlaced content, merge the reference indexes.
> 2) When decoding a slice, use the slice's reference picture if present
> and fill the reference list for the slice.
> 3) Enqueue the slice to decoding context in hardware
> 4) Keep the hardware buffer if it's referred.
>
>
> In the attached patch, I've renamed the former function to
> prepare_vaapi_reference_frame_array, as it only fill the frame's ref
> array with per-frame flags.
> I've added a comment, so it's more clear too.
>
>
> Disclaimer:
> I haven't dug into VA internals & implementation, so I don't know WHY it
> needs to merge the reference list for interlaced content (probably a
> hardware limit?).
> However, the VA header states (from
> http://freedesktop.org/wiki/Software/vaapi ) :
>
> /* H.264 Picture Parameter Buffer */
> /*
>  * For each picture, and before any slice data, a single
>  * picture parameter buffer must be send.
>  */
> typedef struct _VAPictureParameterBufferH264
> {
>     VAPictureH264 CurrPic;
>     VAPictureH264 ReferenceFrames[16];  /* in DPB */
> [...]
>
>

> I haven't written this code (Gwenole has), I'm only trying to use it and
> thought it was a shame that only H264 missed in the trunk.

Thats because the VAAPI-h264 code is a unreadable probably buggy mess, the
other wrapers where of higher quality


[...]
> +/** Initialize an empty VAAPI picture.

> + *  VAAPI requires a fixed size reference picture array.

well, and what is this supposed to tell us in the doxy of this function?

> + */
> +static void init_vaapi_pic(VAPictureH264 *va_pic)
> +{
> +    va_pic->picture_id          = 0xffffffff;
> +    va_pic->flags               = VA_PICTURE_H264_INVALID;
> +    va_pic->TopFieldOrderCnt    = 0;
> +    va_pic->BottomFieldOrderCnt = 0;
> +}


> +
> +/** Translate an FFmpeg Picture into its VAAPI form. 
> + *  @param[out] va_pic          A pointer to VAAPI's own picture struct
> + *  @param[in]  pic             A pointer to the FFmpeg picture struct to convert
> + *  @param[in]  pic_structure   The picture field type (as defined in mpegvideo.h),
> + *                              supersede pic's field type if nonzero.
> + */

> +static void fill_vaapi_pic(VAPictureH264  *va_pic, 
> +                           Picture        *pic, 

trailing whitespace


> +                           int             pic_structure)
> +{
> +    if (pic_structure == 0)
> +        pic_structure = pic->reference;
> +
> +    va_pic->picture_id = ff_vaapi_get_surface(pic);
> +    va_pic->frame_idx  = pic->long_ref ? pic->pic_id : pic->frame_num;
> +
> +    va_pic->flags = 0;
> +    if (pic_structure != PICT_FRAME) {
> +        if (pic_structure & PICT_TOP_FIELD)
> +            va_pic->flags |= VA_PICTURE_H264_TOP_FIELD;

> +        if (pic_structure & PICT_BOTTOM_FIELD)
> +            va_pic->flags |= VA_PICTURE_H264_BOTTOM_FIELD;

is that equivalent to an else ?
if so it should be one


> +    }
> +    if (pic->reference) {

> +        if (pic->long_ref)
> +            va_pic->flags |= VA_PICTURE_H264_LONG_TERM_REFERENCE;
> +        else

if{
}else
(makes future patches smaller)


> +            va_pic->flags |= VA_PICTURE_H264_SHORT_TERM_REFERENCE;
> +    }
> +
> +    va_pic->TopFieldOrderCnt = 0;
> +    if ((pic_structure & PICT_TOP_FIELD) && pic->field_poc[0] != INT_MAX)
> +        va_pic->TopFieldOrderCnt = pic->field_poc[0];
> +
> +    va_pic->BottomFieldOrderCnt = 0;
> +    if ((pic_structure & PICT_BOTTOM_FIELD) && pic->field_poc[1] != INT_MAX)
> +        va_pic->BottomFieldOrderCnt = pic->field_poc[1];
> +}
> +
> +/** Decoded picture buffer. */
> +typedef struct DPB {
> +    unsigned int   size;
> +    unsigned int   max_size;
> +    VAPictureH264 *pics;
> +} DPB;
> +
> +/** Append picture to the decoded picture buffer, in a VAAPI form that 
> + *  merges the second field picture attributes with the first, if available.
> + *  The decoded picture buffer's size must be large enough 
> + *  to receive the new VAAPI picture object.
> + */
> +static int append_to_vaapi_decoded_pic_buf(DPB *dpb, Picture *ff_pic)
> +{
> +    unsigned int i;
> +
> +    if (dpb->size >= dpb->max_size)
> +        return -1;
> +
> +    for (i = 0; i < dpb->size; i++) {
> +        VAPictureH264 * const pic = &dpb->pics[i];
> +        if (pic->picture_id == ff_vaapi_get_surface(ff_pic)) {

> +            VAPictureH264 new_pic;

this variable name is poor, we have ff_pic that is a differnt type ...
really, for the patch to be accepted the vaapi vs. lavc code must be much
clearer seperated and without any exception
as is the code is not readable.
and ff_ is used as a prefix for private global things in lav* its a poor
choice here as well

and the function above uses pic as a Picture type and va_pic as what pic
is used here ...
thats just a mess

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20090608/139568b4/attachment.pgp>