[FFmpeg-devel] [PATCH v3] swscale: Add support for unscaled 8-bit Packed RGB -> Planar RGB

[Derek Buitenhuis]

On 2013-02-23 9:24 PM, Michael Niedermayer wrote:
>> +    dest[0] = dst[0];
>> +    dest[1] = dst[1];
>> +    dest[2] = dst[2];
> 
> unneeded
> or you can drop all the 102/201 stuff and do a
> 
> dest[0] = dst[  2*isRGB];
> dest[1] = dst[1];
> dest[2] = dst[2-2*isRGB];
> 
> inc_size = 3 + hasalpha
> 
> 4 lines instead of 30

What does this accomplish other than making the code more
obtuse and harder to understand? I prefer to err on the
side of simple, explicit code. This is neither.

>> +    if (c->dstFormat != PIX_FMT_GBRP) {
>> +        av_log(c, AV_LOG_ERROR, "unsupported planar RGB conversion %s -> %s\n",
>> +               av_get_pix_fmt_name(c->srcFormat),
>> +               av_get_pix_fmt_name(c->dstFormat));
>> +        return srcSliceH;
>> +    }
> 
> this can only happen if the code is buggy thus should be not here or
> a av_assert

Indeed. Fixed

>> +    default:
>> +        av_log(c, AV_LOG_ERROR,
>> +               "unsupported planar RGB conversion %s -> %s\n",
>> +               av_get_pix_fmt_name(c->srcFormat),
>> +               av_get_pix_fmt_name(c->dstFormat));
>> +    }
> 
> similar issue, unneeded or av_assert
> IMHO av_assert would be better from these 2 options

av_assert likely isn't even needed at all, since it's blatantly impossible...


>> +    if (av_pix_fmt_descriptors[srcFormat].comp[0].depth_minus1 == 7 &&
>> +        isPackedRGB(srcFormat) && dstFormat == AV_PIX_FMT_GBRP)
>> +        c->swScale = rgbToPlanarRgbWrapper;
> 
> this would break some msvc system again probably
> you need av_pix_fmt_desc_get()

You'd think, given that I did a bunch of work on MSVC, I'd have
caught this bug...

Will fix.

- Derek