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

[Cyril Russo]

Diego Biurrun a ?crit :
> On Wed, May 20, 2009 at 10:06:32AM +0200, Cyril Russo wrote:
>   
>> --- libavcodec/vaapi_h264.c	(r??vision 0)
>> +++ libavcodec/vaapi_h264.c	(r??vision 0)
>> @@ -0,0 +1,338 @@
>> +
>> +/** Translate an FFmpeg Picture into its VA API form. 
>> +    @param[out] va_pic          A pointer on a lib VA's own picture struct
>>     
>
> A pointer is never "on" but "to" something, more instances below.
>   
Fixed everwhere.
>   
>> +    @param[in]  pic             A pointer the ffmpeg picture struct to convert
>>     
>
> pointer to?
>   
Ditto.
>> +static void vaapi_h264_fill_vaapi_picture(VAPictureH264 * va_pic, 
>> +                                          Picture *       pic, 
>> +                                          int             pic_structure)
>>     
>
> The * placement is inconsistent with the rest of the code, same all over.
>
>   
Fixed everywhere.
>> +/** Append Picture into the decoded picture buffer, in a VA API form so that to 
>> +    merge the second field picture attributes, if any.
>>     
>
> "so that to merge" is not English and I am not sure what you mean here.
>
>   
Was initial comment approved earlier.
Anyway, I haven't written this comment (Gwenole did), but I've fixed it.
>> +    The decodec picture buffer's size must be large enough 
>> +    to receive the new VA API picture object */
>>     
>
> End sentences in periods.
>   
My mistake.
Done.
>   
>> +/** Initialize VAPictureParameterBufferH264.ReferenceFrames[] array
>> +    from its ffmpeg counterpart. */
>>     
>
> nit: I think all the Doxygen comments would look nicer with * at the
> left, it's what we do in the rest of FFmpeg.
>
>   
Ok, done everywhere on multi line comment.
>> +static int vaapi_h264_fill_ReferenceFrames(VAPictureParameterBufferH264 *pic_param, H264Context *h)
>>     
>
> nit: long line
>   
Split
>   
>> +/** Initialize VA API reference picture lists from ffmpeg reference picture list.
>>     
>
> from the
>
> Please capitalize FFmpeg properly unless you are referrring to the
> ffmpeg binary, same in other places.
>
>   
Done too.
>> +    @param[out] RefPicList  The lib VA's internal reference picture list
>>     
>
> What is "the lib VA"?
>
>   
VA API stands for Video Acceleration API.
It uses a library usually called libva. There is a already a check for 
this lib in configure.
This lib abstracts the video structures so it can be understood by both 
hardware driver and software lib's client.
The comment was only refering to this (required) conversion step.
I've changed all "lib VA" to a common "VAAPI" name, so it's more 
consistant with the other comments.

>> +    @param[out] luma_weight_flag    Set to the luma weight flags used
>> +    @param[out] luma_weight         The final luma weight to use
>> +    @param[out] luma_offset         The final luma offset to use 
>> +    @param[out] chroma_weight_flag  Set to the chroma weight flags used 
>> +    @param[out] chroma_weight       The final chroma weight to use
>> +    @param[out] chroma_offset       The final chroma offset to use */
>>     
>
> Why do you use "Set to" in the explanation of a few parameters?  Seems
> unnecessary..
>   
It's hard to get a small sentence without repeating the same information 
twice.
"@param[out]" says that the flag will be set on output, but doesn't say 
to what.
The comment said the flag will be set to luma weight flag.

VAAPI requires luma and chroma table to be expanded (so both default and 
inferred values must be in the final table).
It's not the case of FFmpeg.


I've changed to "The VAAPI's plain luma weight flag" and the comment 
below to:
"The VAAPI's plain luma weight table"

I've also added a small line to explain why we need this step.
>   
>> +/** End an hardware decoding based frame. */
>>     
>
> a hardware
>   
Done. An hour but a hardware.
Never got used to which h-prefixed words needs "an" or "a".
>   
>> +/** Decode the given H264 slice with VAAPI. */
>>     
>
> H.264
>   
Done.
> Diego
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>
>
>   

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