[FFmpeg-devel] [PATCH] swscale: fix gbrap16 alpha channel issues

[James Cowgill]

Hi,

On 02/08/17 14:18, Michael Niedermayer wrote:
> On Tue, Aug 01, 2017 at 02:46:22PM +0100, James Cowgill wrote:
>> Fixes filter-pixfmts-scale test failing on big-endian systems due to
>> alpSrc not being cast to (const int32_t**).
>>
>> Also fixes distortions in the output alpha channel values by copying the
>> alpha channel code from the rgba64 case found elsewhere in output.c.
>>
>> Fixes ticket 6555.
>>
>> Signed-off-by: James Cowgill <James.Cowgill at imgtec.com>
>> ---
>>  libswscale/output.c                 | 15 ++++++++-------
>>  tests/ref/fate/filter-pixfmts-scale |  4 ++--
>>  2 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/libswscale/output.c b/libswscale/output.c
>> index 9774e9f327..8e5ec0a256 100644
>> --- a/libswscale/output.c
>> +++ b/libswscale/output.c
>> @@ -2026,17 +2026,18 @@ yuv2gbrp16_full_X_c(SwsContext *c, const int16_t *lumFilter,
>>                      const int16_t **lumSrcx, int lumFilterSize,
>>                      const int16_t *chrFilter, const int16_t **chrUSrcx,
>>                      const int16_t **chrVSrcx, int chrFilterSize,
>> -                    const int16_t **alpSrc, uint8_t **dest,
>> +                    const int16_t **alpSrcx, uint8_t **dest,
>>                      int dstW, int y)
>>  {
>>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(c->dstFormat);
>>      int i;
>> -    int hasAlpha = (desc->flags & AV_PIX_FMT_FLAG_ALPHA) && alpSrc;
>> +    int hasAlpha = (desc->flags & AV_PIX_FMT_FLAG_ALPHA) && alpSrcx;
>>      uint16_t **dest16 = (uint16_t**)dest;
>>      const int32_t **lumSrc  = (const int32_t**)lumSrcx;
>>      const int32_t **chrUSrc = (const int32_t**)chrUSrcx;
>>      const int32_t **chrVSrc = (const int32_t**)chrVSrcx;
>> -    int A = 0; // init to silence warning
>> +    const int32_t **alpSrc  = (const int32_t**)alpSrcx;
> 
>> +    int A = 0xFFFF << 14;
> 
> unused value

The initial value of A is unused in the old code, but not in the new code.

>>  
>>      for (i = 0; i < dstW; i++) {
>>          int j;
>> @@ -2059,13 +2060,13 @@ yuv2gbrp16_full_X_c(SwsContext *c, const int16_t *lumFilter,
>>          V >>= 14;
>>  
>>          if (hasAlpha) {
>> -            A = 1 << 18;
>> +            A = -0x40000000;
> 
> where does this value come from ?
> it looks copy and pasted from luma, but alpha does not have a black
> level offset as its not luminance

I confess I only know the basics of how these functions work. On the
basis that yuv2gbrp_full_X_c looks like it copies yuv2rgb_X_c_template,
and I would have thought the rgb and gbr cases should be similar, I
copied a number of things from yuv2rgba64_full_X_c_template into this
function. That value and all of the modifications inside the for loop
come from there.

>>  
>>              for (j = 0; j < lumFilterSize; j++)
>>                  A += alpSrc[j][i] * lumFilter[j];
>>  
>> -            if (A & 0xF8000000)
>> -                A =  av_clip_uintp2(A, 27);
>> +            A >>= 1;
>> +            A += 0x20002000;
>>          }
>>  
>>          Y -= c->yuv2rgb_y_offset;
>> @@ -2083,7 +2084,7 @@ yuv2gbrp16_full_X_c(SwsContext *c, const int16_t *lumFilter,
>>          dest16[1][i] = B >> 14;
>>          dest16[2][i] = R >> 14;
>>          if (hasAlpha)
>> -            dest16[3][i] = A >> 11;
>> +            dest16[3][i] = av_clip_uintp2(A, 30) >> 14;
> 
> why do you move the cliping code here, this seems unneeded
> outside the removed if()

This is where the clipping code in yuv2rgba64_full_X_c_template is, and
in that function, the value of A is not clipped - only the value stored
in dest.

Thanks,
James