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

[Michael Niedermayer]

On Wed, Aug 02, 2017 at 03:32:04PM +0100, James Cowgill wrote:
> 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.

IIRC all uses are under hasAlpha and it is writen to in that case first


> 
> >>  
> >>      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.
[...]
> 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.

hmm, ok, on a 2nd look this is probably correct

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170803/e09d0db1/attachment.sig>