Discussion:
[FFmpeg-devel] [PATCH] Fixed PNG decoding with Paeth filter for RGBA 64-bits.
Dominique Leroux
2014-12-05 00:20:31 UTC
Permalink
Found and fixed an artifact in the last column of decoded RGBA 64-bits PNG images.

The code was dealing with a SIMD-optimized version of the function without taking into account that we can have RGB/RGBA images that are respectively 6 or 8 bytes per pixel (not just 3 or 4).

Dominique
Christophe Gisquet
2014-12-05 09:29:49 UTC
Permalink
Hi,
Post by Dominique Leroux
Found and fixed an artifact in the last column of decoded RGBA 64-bits PNG images.
The code was dealing with a SIMD-optimized version of the function without taking into account that we can have RGB/RGBA images that are respectively 6 or 8 bytes per pixel (not just 3 or 4).
First, what version are you working on? Ronald included a fix in
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=af79a0c48a41fd99b674b39ac509ae442974715d
that would then be incomplete?

If true, then it probably means we don't have a "fate" test for it (ie
a means to validate ffmpeg's output against a known "good"
checksum/behaviour), or that it is unaffected by the bug.

In any case, could you share a sample file exhibiting the issue? I
guess cropping to the last few lines would be fine from a quick glance
at your patch. But I haven't studied the issue to actually validate
it.

Thank you,
--
Christophe
Dominique Leroux
2014-12-05 15:44:09 UTC
Permalink
Hi Cristophe,

Before submitting yesterday evening, I pulled the latest changes from master and the problem is still present.

My situation happens with RGBA64. The structure of the existing C code was meant to enter the optimized path with a buffer size that is compatible with SIMD constraints. So I just generalized the buffer resizing so it takes any bpp into account (and not just bpp=3 or bpp=4 like before).

Here are sample files that are 4 lines long: the original and the corrupted output. The corruption is on the rightmost edge, where last 3 lines have the expected pink replaced by yellow.

With my fix, the resulting output is as as the original. The resulting file layout differs, however: the original has one single IDAT chunk for all 4 lines, whereas the output files have one IDAT chunk per line (hence the bigger size). Maybe this is part of the reason it went unnoticed for so long as I don't think it is possible to generate such a png file with ffmpeg and the corruption does not happen on the first line.

Thanks for looking at this,

Dominique

-----Original Message-----
From: ffmpeg-devel-***@ffmpeg.org [mailto:ffmpeg-devel-***@ffmpeg.org] On Behalf Of Christophe Gisquet
Sent: Friday, December 05, 2014 4:30 AM
To: FFmpeg development discussions and patches
Subject: Re: [FFmpeg-devel] [PATCH] Fixed PNG decoding with Paeth filter for RGBA 64-bits.

Hi,
Post by Dominique Leroux
Found and fixed an artifact in the last column of decoded RGBA 64-bits PNG images.
The code was dealing with a SIMD-optimized version of the function without taking into account that we can have RGB/RGBA images that are respectively 6 or 8 bytes per pixel (not just 3 or 4).
First, what version are you working on? Ronald included a fix in http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=af79a0c48a41fd99b674b39ac509ae442974715d
that would then be incomplete?

If true, then it probably means we don't have a "fate" test for it (ie a means to validate ffmpeg's output against a known "good"
checksum/behaviour), or that it is unaffected by the bug.

In any case, could you share a sample file exhibiting the issue? I guess cropping to the last few lines would be fine from a quick glance at your patch. But I haven't studied the issue to actually validate it.

Thank you,
--
Christophe
Michael Niedermayer
2014-12-07 04:39:16 UTC
Permalink
Post by Dominique Leroux
Found and fixed an artifact in the last column of decoded RGBA 64-bits PNG images.
The code was dealing with a SIMD-optimized version of the function without taking into account that we can have RGB/RGBA images that are respectively 6 or 8 bytes per pixel (not just 3 or 4).
Dominique
Changelog | 1 +
libavcodec/pngdec.c | 16 ++++++++++++----
libavcodec/pngdsp.c | 7 +++++--
libavcodec/pngdsp.h | 5 +++++
libavcodec/x86/pngdsp_init.c | 17 +++++++++++------
5 files changed, 34 insertions(+), 12 deletions(-)
a3c13eb0d8d35a5f23a05c3005b7792893f1dc2f 0001-Fixed-PNG-decoding-with-Paeth-filter-for-RGBA-64-bit.patch
From 5d3b88d2a9bad1f9e9431b6244493cbc6dda5701 Mon Sep 17 00:00:00 2001
Date: Thu, 4 Dec 2014 19:05:44 -0500
Subject: [PATCH] Fixed PNG decoding with Paeth filter for RGBA 64-bits.
I noticed an artifact on the resulting image when passing a RGBA 64-bits PNG
through the copy filter. The last column, although obviously "derived" from the
original image's last column (i.e. not completely garbage), had way too much
yellow in it; a telltale sign of an off-by-X somewhere. Original scenario used
the scale filter, but taking it out was still exhibiting the problem.
./ffmpeg.exe -i original_rgba64.png -vf copy result.png
and inspecting the result image with a viewer. I realized the image had some
lines filtered with Paeth, and only those lines would get the artifact on their
rightmost pixel.
In libavcodec/pngdec.c, png_filter_row() has buffer-sizing arithmetic for
feeding the Paeth prediction function with a buffer that has a multiple of 4
bytes. This is done so the optimized version of the algorithm, using MMX/SSE3,
can work on 4 bytes at a time without causing a write past the end of the buffer
when writing back into the result buffer. This arithmetic assumed that
situations where the buffer is improperly sized were limited to RGB 24-bits
(bpp, or bytes per pixel, = 3) and that all other formats had buffers properly
sized for SIMD, i.e. a multiple of 4.
. bpp == 8 (RGBA 64-bits, my case) also has the proper size for SIMD. But it
got handled as if it had a bpp of 3 because the code was testing for bpp == 4
(with ternary operator's operands reversed).
. bpp == 6 (RGB 48-bits) can also cause writes past the end of the buffer when
handling the last 2 of 6 bytes for an image that has an odd number of columns.
So I am reformulating the SIMD-adjusted width to ignore the trailing 3 (or less)
bytes when the total width is not a multiple of 4. These bytes are fed to the
unoptimized version of the algorithm.
I was tempted to apply this to bpp == 1 as well (i.e. take out the bpp > 2 test
in the enclosing if). But looking at pngdec.c's history, this has been done
earlier and differences between the results of the optimized and non-optimized
versions of the algorithm were observed. So I am leaving this untouched.
It then looked to me that knowing the SIMD stride requirements of the x86
platform implementation was a bit of a long-distance coupling between the
platform-independent pngdec.c and the x86-specific x86/pngdsp.asm. So I
SIMD stride as well as a mask for adjusting the buffer size.
---
Changelog | 1 +
libavcodec/pngdec.c | 16 ++++++++++++----
libavcodec/pngdsp.c | 7 +++++--
libavcodec/pngdsp.h | 5 +++++
libavcodec/x86/pngdsp_init.c | 17 +++++++++++------
5 files changed, 34 insertions(+), 12 deletions(-)
this is not correct
iam a bit tired so i just quickly read your elaborate text but the asm
works in steps of bpp not steps of 4 and writes 4 bytes at a time
you seem to assume the asm is simpler and more systematic than it
actually is, also theres are bugs in the asm i think
ive a fix for some of this locally but id like to test it more before
pushing

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
Michael Niedermayer
2014-12-07 11:55:27 UTC
Permalink
Hi

On Sun, Dec 07, 2014 at 05:39:16AM +0100, Michael Niedermayer wrote:
[...]
Post by Michael Niedermayer
Post by Dominique Leroux
Changelog | 1 +
libavcodec/pngdec.c | 16 ++++++++++++----
libavcodec/pngdsp.c | 7 +++++--
libavcodec/pngdsp.h | 5 +++++
libavcodec/x86/pngdsp_init.c | 17 +++++++++++------
5 files changed, 34 insertions(+), 12 deletions(-)
this is not correct
iam a bit tired so i just quickly read your elaborate text but the asm
works in steps of bpp not steps of 4 and writes 4 bytes at a time
you seem to assume the asm is simpler and more systematic than it
actually is, also theres are bugs in the asm i think
ive a fix for some of this locally but id like to test it more before
pushing
patchset posted, please take a look and review / check / confirm if
this fixes all issues you saw

Thanks

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
Dominique Leroux
2014-12-08 16:04:56 UTC
Permalink
Hi Michael,

Yes, this does fix the issues I saw with RGBA64.

Thanks,

Dominique

-----Original Message-----
From: Michael Niedermayer [mailto:***@gmx.at]
Sent: Sunday, December 07, 2014 6:55 AM
To: FFmpeg development discussions and patches
Cc: Dominique Leroux
Subject: Re: [FFmpeg-devel] [PATCH] Fixed PNG decoding with Paeth filter for RGBA 64-bits.

Hi

On Sun, Dec 07, 2014 at 05:39:16AM +0100, Michael Niedermayer wrote:
[...]
Post by Michael Niedermayer
Post by Dominique Leroux
Changelog | 1 +
libavcodec/pngdec.c | 16 ++++++++++++----
libavcodec/pngdsp.c | 7 +++++--
libavcodec/pngdsp.h | 5 +++++
libavcodec/x86/pngdsp_init.c | 17 +++++++++++------
5 files changed, 34 insertions(+), 12 deletions(-)
this is not correct
iam a bit tired so i just quickly read your elaborate text but the asm
works in steps of bpp not steps of 4 and writes 4 bytes at a time you
seem to assume the asm is simpler and more systematic than it actually
is, also theres are bugs in the asm i think ive a fix for some of this
locally but id like to test it more before pushing
patchset posted, please take a look and review / check / confirm if this fixes all issues you saw

Thanks

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

Many things microsoft did are stupid, but not doing something just because microsoft did it is even more stupid. If everything ms did were stupid they would be bankrupt already.
Michael Niedermayer
2014-12-08 17:39:28 UTC
Permalink
Post by Dominique Leroux
Hi Michael,
Yes, this does fix the issues I saw with RGBA64.
patches applied

thanks

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
Loading...