| Message ID | 20200915021446.48394-1-mindmark@gmail.com |
|---|---|
| State | Accepted |
| Commit | 8ddcbebc3f869def7819697d7f4f85b94308c570 |
| Headers | show |
| Series | [FFmpeg-devel] libavcodec/exr: fix incorrect translation of denorm mantissa | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Mon, Sep 14, 2020 at 7:14 PM <mindmark@gmail.com> wrote: > From: Mark Reid <mindmark@gmail.com> > > Hi, > This fixes a very subtle error thats hard notice until up unpremultiply a > image. > > This loop is suppose to stop at first 1, instead was stoping at first 0 > The comment is correct through! > > openexrs implementation is very similar. > > https://github.com/AcademySoftwareFoundation/openexr/blob/master/IlmBase/Half/toFloat.cpp#L85 > > not all the exr tests needed to be fixed because only some have denorm > values > > --- > libavcodec/exr.c | 2 +- > tests/ref/fate/exr-rgba-multiscanline-half-b44 | 2 +- > tests/ref/fate/exr-slice-raw | 2 +- > tests/ref/fate/exr-slice-rle | 2 +- > tests/ref/fate/exr-slice-zip1 | 2 +- > tests/ref/fate/exr-slice-zip16 | 2 +- > 6 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/exr.c b/libavcodec/exr.c > index 829d38143d..216d216785 100644 > --- a/libavcodec/exr.c > +++ b/libavcodec/exr.c > @@ -201,7 +201,7 @@ static union av_intfloat32 exr_half2float(uint16_t hf) > mantissa <<= 1; > exp = HALF_FLOAT_MIN_BIASED_EXP_AS_SINGLE_FP_EXP; > // check for leading 1 in denorm mantissa > - while ((mantissa & (1 << 10))) { > + while (!(mantissa & (1 << 10))) { > // for every leading 0, decrement single precision > exponent by 1 > // and shift half-float mantissa value to the left > mantissa <<= 1; > diff --git a/tests/ref/fate/exr-rgba-multiscanline-half-b44 > b/tests/ref/fate/exr-rgba-multiscanline-half-b44 > index 964bf2e65e..24525b92a7 100644 > --- a/tests/ref/fate/exr-rgba-multiscanline-half-b44 > +++ b/tests/ref/fate/exr-rgba-multiscanline-half-b44 > @@ -3,4 +3,4 @@ > #codec_id 0: rawvideo > #dimensions 0: 935x251 > #sar 0: 1/1 > -0, 0, 0, 1, 3754960, 0x4d48a1b2 > +0, 0, 0, 1, 3754960, 0x8d9af112 > diff --git a/tests/ref/fate/exr-slice-raw b/tests/ref/fate/exr-slice-raw > index c7096e4d2a..1e7d3825ea 100644 > --- a/tests/ref/fate/exr-slice-raw > +++ b/tests/ref/fate/exr-slice-raw > @@ -3,4 +3,4 @@ > #codec_id 0: rawvideo > #dimensions 0: 587x675 > #sar 0: 1/1 > -0, 0, 0, 1, 6339600, 0x4f2b496b > +0, 0, 0, 1, 6339600, 0xda3e31df > diff --git a/tests/ref/fate/exr-slice-rle b/tests/ref/fate/exr-slice-rle > index c7096e4d2a..1e7d3825ea 100644 > --- a/tests/ref/fate/exr-slice-rle > +++ b/tests/ref/fate/exr-slice-rle > @@ -3,4 +3,4 @@ > #codec_id 0: rawvideo > #dimensions 0: 587x675 > #sar 0: 1/1 > -0, 0, 0, 1, 6339600, 0x4f2b496b > +0, 0, 0, 1, 6339600, 0xda3e31df > diff --git a/tests/ref/fate/exr-slice-zip1 b/tests/ref/fate/exr-slice-zip1 > index c7096e4d2a..1e7d3825ea 100644 > --- a/tests/ref/fate/exr-slice-zip1 > +++ b/tests/ref/fate/exr-slice-zip1 > @@ -3,4 +3,4 @@ > #codec_id 0: rawvideo > #dimensions 0: 587x675 > #sar 0: 1/1 > -0, 0, 0, 1, 6339600, 0x4f2b496b > +0, 0, 0, 1, 6339600, 0xda3e31df > diff --git a/tests/ref/fate/exr-slice-zip16 > b/tests/ref/fate/exr-slice-zip16 > index c7096e4d2a..1e7d3825ea 100644 > --- a/tests/ref/fate/exr-slice-zip16 > +++ b/tests/ref/fate/exr-slice-zip16 > @@ -3,4 +3,4 @@ > #codec_id 0: rawvideo > #dimensions 0: 587x675 > #sar 0: 1/1 > -0, 0, 0, 1, 6339600, 0x4f2b496b > +0, 0, 0, 1, 6339600, 0xda3e31df > -- > 2.27.0 > > to see the problem ffmpeg_g -y -i premult_test.exr -vf unpremultiply=inplace=1 out.jpg here is that test file, and a before and after patch pic https://www.dropbox.com/s/pgdxzxo454r95jn/premult_test.zip
diff --git a/libavcodec/exr.c b/libavcodec/exr.c index 829d38143d..216d216785 100644 --- a/libavcodec/exr.c +++ b/libavcodec/exr.c @@ -201,7 +201,7 @@ static union av_intfloat32 exr_half2float(uint16_t hf) mantissa <<= 1; exp = HALF_FLOAT_MIN_BIASED_EXP_AS_SINGLE_FP_EXP; // check for leading 1 in denorm mantissa - while ((mantissa & (1 << 10))) { + while (!(mantissa & (1 << 10))) { // for every leading 0, decrement single precision exponent by 1 // and shift half-float mantissa value to the left mantissa <<= 1; diff --git a/tests/ref/fate/exr-rgba-multiscanline-half-b44 b/tests/ref/fate/exr-rgba-multiscanline-half-b44 index 964bf2e65e..24525b92a7 100644 --- a/tests/ref/fate/exr-rgba-multiscanline-half-b44 +++ b/tests/ref/fate/exr-rgba-multiscanline-half-b44 @@ -3,4 +3,4 @@ #codec_id 0: rawvideo #dimensions 0: 935x251 #sar 0: 1/1 -0, 0, 0, 1, 3754960, 0x4d48a1b2 +0, 0, 0, 1, 3754960, 0x8d9af112 diff --git a/tests/ref/fate/exr-slice-raw b/tests/ref/fate/exr-slice-raw index c7096e4d2a..1e7d3825ea 100644 --- a/tests/ref/fate/exr-slice-raw +++ b/tests/ref/fate/exr-slice-raw @@ -3,4 +3,4 @@ #codec_id 0: rawvideo #dimensions 0: 587x675 #sar 0: 1/1 -0, 0, 0, 1, 6339600, 0x4f2b496b +0, 0, 0, 1, 6339600, 0xda3e31df diff --git a/tests/ref/fate/exr-slice-rle b/tests/ref/fate/exr-slice-rle index c7096e4d2a..1e7d3825ea 100644 --- a/tests/ref/fate/exr-slice-rle +++ b/tests/ref/fate/exr-slice-rle @@ -3,4 +3,4 @@ #codec_id 0: rawvideo #dimensions 0: 587x675 #sar 0: 1/1 -0, 0, 0, 1, 6339600, 0x4f2b496b +0, 0, 0, 1, 6339600, 0xda3e31df diff --git a/tests/ref/fate/exr-slice-zip1 b/tests/ref/fate/exr-slice-zip1 index c7096e4d2a..1e7d3825ea 100644 --- a/tests/ref/fate/exr-slice-zip1 +++ b/tests/ref/fate/exr-slice-zip1 @@ -3,4 +3,4 @@ #codec_id 0: rawvideo #dimensions 0: 587x675 #sar 0: 1/1 -0, 0, 0, 1, 6339600, 0x4f2b496b +0, 0, 0, 1, 6339600, 0xda3e31df diff --git a/tests/ref/fate/exr-slice-zip16 b/tests/ref/fate/exr-slice-zip16 index c7096e4d2a..1e7d3825ea 100644 --- a/tests/ref/fate/exr-slice-zip16 +++ b/tests/ref/fate/exr-slice-zip16 @@ -3,4 +3,4 @@ #codec_id 0: rawvideo #dimensions 0: 587x675 #sar 0: 1/1 -0, 0, 0, 1, 6339600, 0x4f2b496b +0, 0, 0, 1, 6339600, 0xda3e31df
From: Mark Reid <mindmark@gmail.com> Hi, This fixes a very subtle error thats hard notice until up unpremultiply a image. This loop is suppose to stop at first 1, instead was stoping at first 0 The comment is correct through! openexrs implementation is very similar. https://github.com/AcademySoftwareFoundation/openexr/blob/master/IlmBase/Half/toFloat.cpp#L85 not all the exr tests needed to be fixed because only some have denorm values --- libavcodec/exr.c | 2 +- tests/ref/fate/exr-rgba-multiscanline-half-b44 | 2 +- tests/ref/fate/exr-slice-raw | 2 +- tests/ref/fate/exr-slice-rle | 2 +- tests/ref/fate/exr-slice-zip1 | 2 +- tests/ref/fate/exr-slice-zip16 | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-)