Re: [Qemu-devel] [PATCH v2] ui/vnc: VA API based H.264 encoding for VNC

[Blue Swirl]

On Fri, Feb 22, 2013 at 2:31 PM, Verbeiren, David
<address@hidden> wrote:
> On Wed, Feb 13, 2013 at 22:16, Blue Swirl <address@hidden> wrote:
>>> +    /* RGBA => NV12 */
>>> +    for (i = 0; i < h264->pic_height; ++i) {
>>> +        dst_y = (pdst + image.offsets[0]) + i*image.pitches[0];
>>> +        dst_uv = dst_uv_line;
>>> +        s = psrc;
>>> +        for (j = 0; j < h264->pic_width; j++) {
>>> +            *dst_y++ = ((66*s[0] + 129*s[1] + 25*s[2] + 128) >> 8) + 16;
>>
>> Please add spaces around operators, scripts/checkpatch.pl may find
>> problems like this. The magic constants should be replaced by enums or
>> #defines.
>>
>>> +            /* NV12 means subsampling by 2 in x and y */
>>> +            if ((0 == i%2) && (0 == j%2)) {
>>> +                *dst_uv++ = ((112*s[0] - 94*s[1] - 18*s[2] + 128) >> 8) + 
>>> 128;
>>> +                *dst_uv++ = ((-38*s[0] - 74*s[1] + 112*s[2] + 128) >> 8) + 
>>> 128;
>
> Instead of creating 9 artificial names for the factors, I'd rather create the
> following 3 macros:
>         #define RGB_TO_Y(r, g, b) ((( 66 * r + 129 * g +  25 * b + 128) >> 8) 
> + 16)
>         #define RGB_TO_U(r, g, b) (((112 * r -  94 * g -  18 * b + 128) >> 8) 
> + 128)
>         #define RGB_TO_V(r, g, b) (((-38 * r -  74 * g + 112 * b + 128) >> 8) 
> + 128)
>
> I think that will make the code more readable and will make it obvious
> what the magic numbers are for. Would that be ok?

That's even better. Inline functions are preferred to macros though.

>
> I'll follow your recommendation for the other items you brought up.
>
> -David
> Intel Corporation NV/SA
> Kings Square, Veldkant 31
> 2550 Kontich
> RPM (Bruxelles) 0415.497.718.
> Citibank, Brussels, account 570/1031255/09
>
> This e-mail and any attachments may contain confidential material for the 
> sole use of the intended recipient(s). Any review or distribution by others 
> is strictly prohibited. If you are not the intended recipient, please contact 
> the sender and delete all copies.