On Fri, Feb 22, 2013 at 2:31 PM, Verbeiren, David
<david.verbei...@intel.com> wrote:
> On Wed, Feb 13, 2013 at 22:16, Blue Swirl <blauwir...@gmail.com> 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.
