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

[Verbeiren, David]

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?

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.