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

[Cyril Russo]

Michael Niedermayer a ?crit :
>
>
> [...]
>
>   
>> +/** Initialize an empty VAAPI picture.
>> + *  VAAPI requires a fixed size reference picture array. */
>> +static void init_picture(VAPictureH264 *va_pic)
>>     
>
> init_vapic(ture)
>
>
>   
>> +{
>> +    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. */
>>     
>
> this is much better, thanks
>
>
>   
>> +static void fill_vaapi_pic(VAPictureH264  *va_pic, 
>> +                           Picture        *pic, 
>> +                           int             pic_structure)
>>     
>
> the function name is inconsistant to init_picture() (vaapi_pic vs picture)
>   

I've removed all prefixes (except for those that are exported (ie: 
vaapi_codecName_start / decode / end), so it's exactly like other 
vaapi_codec files).
Now every method that modifiy a vaapi structure get its name updated:
init_vaapi_pic
fill_vaapi_pic
fill_vaapi_pic_reference_list etc...
>
> [...]
>   
>> +/** Fill VAAPI reference picture lists.
>> + *  @param[out] RefPicList  VAAPI internal reference picture list
>> + *  @param[in]  ref_list    A pointer to the FFmpeg reference list
>>     
>
>   
>> + *  @param[in]  ref_count   The number of reference pictures in ref_list */    
>>     
>
> trailing whitespace and */ should be on a line of its own because that makes
> future patches smaller (they dont have to move it down)
>   

Done (everywhere)

>
>   
>> +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...)
- other prefix have been removed, and I've renamed the functions to 
prefix vaapi on the structure that's modified instead
(ex: init_picture => init_vaapi_pic, fill_reference_pic_list => 
fill_vaapi_reference_pic_list, fill_vaapi_pic => kept the same...)

I think it more obvious this way.

>
> [...]
>   
>> +/** End a hardware decoding based frame. */
>>     
>
> sounds a little un-english
>   

/** Finish hardware decoding of the frame */ ?

Cyril

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ffmpeg.hwaccel.vaapi.h264.16.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090604/476d4eb5/attachment.asc>