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

[Cyril Russo]

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.

Cyril

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ffmpeg.hwaccel.vaapi.h264.15.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090602/65dd51a6/attachment.txt>