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

[Michael Niedermayer]

On Tue, Jun 02, 2009 at 04:03:50PM +0200, Cyril Russo wrote:
> Michael Niedermayer a ?crit :
>>
>> please document the convention used for function names
>> which are part of our wraper and which are part of VAAPI
>> In that sense is it really needed to have h264 and vaapi prefixes for 
>> static
>> function names in a file that is specific to these?
>>
>>   
> I've removed the prefix for the static functions used to convert the data 
> type between FFmpeg and VAAPI.
>
> The h264 prefix are modified to vaapi_h264 to be consistent with other 
> vaapi_*.c files. (vaapi_vc1, vaapi_mpeg4 and others).
> I've added a single line below the copyright notice to explain what the 
> file's code does.
>
> I think it's obvious from other vaapi_'s files, but I'm not the best guy to 
> judge.
>>   
>>> +{
>>> +    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 to use, as 
>>> defined in mpegvideo.h
>>> + *                              (can be 0 to use pic's field type) */
>>>     
>>
>> what does "to use" mean? I dont think thats clear
>>   
> Reformulated.
> During conversion, the pic_structure parameter can be used instead of the 
> pic's internal field type (as VAAPI is not following the same format as 
> FFmpeg).
>>
>>   
>>> +static void vaapi_h264_fill_vaapi_picture(VAPictureH264  *va_pic,
>>>     
>>
>> the function name is not good
>>
>> libav_pic_to_vaapi()
>> might be reasonable
>>
>> [...]
>>   
> There isn't any "libav_" in the code repository, and "av_" look like it's 
> public in the other files.
> I've modified all functions' name like this one to "fill_something" 
> (fill_reference_frame_array, etc...), and added a comment on the top of the 
> file.
>
>>
>>   
>>> +/** Initialize VAPictureParameterBufferH264.ReferenceFrames[] array
>>> + *  from its FFmpeg counterpart. */
>>>     
>> [...]
>>   
>>> +/** Initialize VAAPI reference picture lists from the FFmpeg reference 
>>> picture list.
>>>     
>>
>> hmm, confusing
>>
>> [...]
>>     
> While the former isn't clear, the latter seems logical to me.
> VAAPI and FFmpeg are different code, and we must convert some of the data 
> from FFmpeg to VAAPI.
> Let me know if the attached patch is better.

[...]

> +/** 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)


[...]
> +/** 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)


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


[...]
> +/** End a hardware decoding based frame. */

sounds a little un-english


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

There will always be a question for which you do not know the correct awnser.
-------------- 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/20090604/5644f078/attachment.pgp>