Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"uncompressed" tests fail on various platforms #1256

Closed
fancycode opened this issue Aug 1, 2024 · 19 comments
Closed

"uncompressed" tests fail on various platforms #1256

fancycode opened this issue Aug 1, 2024 · 19 comments

Comments

@fancycode
Copy link
Member

Various uncompressed tests are failing on platforms like powerpc, ppc64 or sparc64 when building for Debian.

Example run:
https://buildd.debian.org/status/fetch.php?pkg=libheif&arch=powerpc&ver=1.18.1-1&stamp=1722512466&raw=0

The following tests FAILED:
	  5 - uncompressed_decode_generic_compression (Failed)
	  6 - uncompressed_decode_mono (Failed)
	  7 - uncompressed_decode_rgb (Failed)
	  8 - uncompressed_decode_rgb16 (Failed)
	  9 - uncompressed_decode_rgb565 (Failed)
	 10 - uncompressed_decode_rgb7 (Failed)
	 11 - uncompressed_decode_ycbcr (Failed)
	 12 - uncompressed_decode_ycbcr420 (Failed)
	 13 - uncompressed_decode_ycbcr422 (Failed)

Other examples available on https://buildd.debian.org/status/package.php?p=libheif (click on the "Build-Attempted" links).

CC @bradh as I think you know probably best about the tests. Thanks!

@bradh
Copy link
Contributor

bradh commented Aug 2, 2024

Given it works on ppc64el and fails on ppc64, my guess would be some endianess issue. I have no idea what though - not even whether the problem is in the tests or in in the codec.

@fancycode
Copy link
Member Author

The code currently checks that uncompressed images are marked as "big endian":

if (uncC->is_components_little_endian()) {
printf("unsupported components LE\n");
return Error(heif_error_Unsupported_feature,
heif_suberror_Unsupported_data_version,
"Uncompressed components_little_endian == 1 is not implemented yet");
}

So I'm thinking the code might only support the internal conversion "big endian image" -> "little endian system". It's using memcpy in a couple of places which might need to be replaced for "big endian image" -> "big endian system"? Just guessing though.

@bradh
Copy link
Contributor

bradh commented Aug 3, 2024

Conceivable, but it looks like there are failures on the 8 bit RGB test, which should not be affected by endianess conversions....

@bradh
Copy link
Contributor

bradh commented Sep 21, 2024

@fancycode Can you re-run on master? I'm not expecting it to work, just that there is a new test that might (or might not) point to the underlying issue.

@fancycode
Copy link
Member Author

Unfortunately I don't have direct access to such machines (yet). The problem occurred when building the official Debian packages - which I can't just update to test some changes.

Hopefully I'll have access soon and will be able to test / debug this further.

@farindk
Copy link
Contributor

farindk commented Sep 23, 2024

I've cross-compiled libheif to PowerPC. The executables will run when qemu-ppc is installed. This allows to reproduce the failing test cases. I'll have a look at it.

Here is the cross-compilation toolchain file:

set(CMAKE_SYSTEM_NAME PPC)

set(CMAKE_C_COMPILER   powerpc-linux-gnu-gcc-12)
set(CMAKE_CXX_COMPILER powerpc-linux-gnu-g++-12)

set(CMAKE_EXE_LINKER_FLAGS "-static")

@farindk
Copy link
Contributor

farindk commented Sep 23, 2024

Found it. The bug is in lines like this:

int val = srcBits.get_bits(entry.bits_per_component_sample);
memcpy(entry.dst_plane + dst_row_offset + dst_column_offset, &val, entry.bytes_per_component_sample);

On BE, this copies the high-order byte of val into the output, which is always 0 for 8 bit components.

It works if I replace it with:

assert(entry.bytes_per_component_sample==1);
uint64_t dst_sample_offset = tile_x * entry.bytes_per_component_sample;
entry.dst_plane[dst_offset + dst_sample_offset] = static_cast<uint8_t>(srcBits.get_bits(entry.bits_per_component_sample));

A clean fix is more complex because entry.bytes_per_component_sample is the number of bytes in the file, but this will not match the size in memory in the general case (e.g. 24bit in the file will be 32bit integers in memory).
This will probably be cleaned up in the course of implementing support for other unci datatypes anyway.

@fancycode
Copy link
Member Author

Great, thanks for testing! I'll leave the uncompressed codec disabled for now in the Debian packaging then, so the behaviour / feature set is consistent across platforms:

https://salsa.debian.org/multimedia-team/libheif/-/commit/55e08a575f76be0ff4d9c1422c5669ef57922e50

@farindk
Copy link
Contributor

farindk commented Oct 29, 2024

@bradh I'm currently trying to fix the endianness issue in the uncompressed codec because it is a blocker for the v1.19.0 release.
I could fix the processing in the codec, but now stumbled on these lines in the tests:

REQUIRE(((int)(img_plane[stride * row + 0])) == 0x8A);
REQUIRE(((int)(img_plane[stride * row + 1])) == 0x4C);
REQUIRE(((int)(img_plane[stride * row + 2])) == 0x8A);
REQUIRE(((int)(img_plane[stride * row + 3])) == 0x4C);
REQUIRE(((int)(img_plane[stride * row + 4])) == 0x8A);
REQUIRE(((int)(img_plane[stride * row + 5])) == 0x4C);
REQUIRE(((int)(img_plane[stride * row + 6])) == 0x8A);

These are per-byte checks, but the image data is 16bits. Thus these tests only work on little-endian systems.
Can you confirm this or am I missing something?

@bradh
Copy link
Contributor

bradh commented Oct 29, 2024

It does appear to be byte order dependent.

Recommended removal of the values check for 1.19.0 and I will try again with a uint16_t version later.

@farindk
Copy link
Contributor

farindk commented Oct 29, 2024

The endianness issues should be fixed now (at least to make the tests run on ppc64).
The uncompressed_decode_generic_compression test is still failing for me, but that is probably because I have no zlib when cross-compiling to ppc64.

@fancycode Can you please retry building the package? Now with WITH_UNCOMPRESSED_CODEC enabled?

@bradh The endianness flags in the uncC header are not taken care of yet. I'll leave this for future work. Currently, it is assuming that all files are stored in big-endian.

@fancycode
Copy link
Member Author

@fancycode Can you please retry building the package? Now with WITH_UNCOMPRESSED_CODEC enabled?

I have successfully build and tested libheif from 474d68b on a ppc64 box (after installing necessary dependencies):

mkdir build
cd build
cmake .. \
  -DWITH_AOM_DECODER_PLUGIN=ON \
  -DWITH_AOM_ENCODER_PLUGIN=ON \
  -DWITH_DAV1D=ON \
  -DWITH_DAV1D_PLUGIN=ON \
  -DWITH_DEFLATE_HEADER_COMPRESSION=ON \
  -DWITH_FFMPEG_DECODER=ON \
  -DWITH_FFMPEG_DECODER_PLUGIN=ON \
  -DWITH_JPEG_DECODER=ON \
  -DWITH_JPEG_DECODER_PLUGIN=ON \
  -DWITH_JPEG_ENCODER=ON \
  -DWITH_JPEG_ENCODER_PLUGIN=ON \
  -DWITH_KVAZAAR=ON \
  -DWITH_KVAZAAR_PLUGIN=ON \
  -DWITH_LIBDE265_PLUGIN=ON \
  -DWITH_LIBSHARPYUV=ON \
  -DWITH_OpenJPEG_DECODER=ON \
  -DWITH_OpenJPEG_DECODER_PLUGIN=ON \
  -DWITH_OpenJPEG_ENCODER=ON \
  -DWITH_OpenJPEG_ENCODER_PLUGIN=ON \
  -DWITH_X265_PLUGIN=ON \
  -DWITH_RAV1E=ON \
  -DWITH_RAV1E_PLUGIN=ON \
  -DWITH_SvtEnc=ON \
  -DWITH_SvtEnc_PLUGIN=ON \
  -DWITH_UNCOMPRESSED_CODEC=ON \
  -DWITH_OpenH264_DECODER_PLUGIN=ON
make -j$(nproc)
LIBHEIF_PLUGIN_PATH=$(pwd)/libheif/plugins/ make test

Plugin details:

=== Summary of compiled codecs ===
libde265 HEVC decoder           : + separate plugin
FFMPEG HEVC decoder (HW acc)    : + separate plugin
x265 HEVC encoder               : + separate plugin
Kvazaar HEVC encoder            : + separate plugin
AOM AV1 decoder                 : + separate plugin
AOM AV1 encoder                 : + separate plugin
Dav1d AV1 decoder               : + separate plugin
SVT AV1 encoder                 : + separate plugin
Rav1e AV1 encoder               : + separate plugin
JPEG decoder                    : + separate plugin
JPEG encoder                    : + separate plugin
OpenH264 decoder                : + separate plugin
OpenJPEG J2K decoder            : + separate plugin
OpenJPEG J2K encoder            : + separate plugin
OpenJPH HT-J2K encoder          : - disabled
uvg266 VVC enc. (experimental)  : - disabled
vvenc VVC enc. (experimental)   : - disabled
vvdec VVC dec. (experimental)   : - disabled

=== Supported formats ===
format        decoding   encoding
AVC             YES        NO 
AVIF            YES        YES
HEIC            YES        YES
JPEG            YES        YES
JPEG2000        YES        YES
JPEG2000-HT     YES        NO 
Uncompressed    YES        YES
VVC             NO         NO 

-- Found LIBSHARPYUV 
libsharpyuv: found
zlib found
-- Did not find Brotli (missing: BROTLI_COMMON_LIB BROTLI_DEC_INCLUDE_DIR BROTLI_DEC_LIB BROTLI_ENC_INCLUDE_DIR BROTLI_ENC_LIB) 
Brotli not found

=== Active input formats for heif-enc ===
JPEG: active
PNG:  active
TIFF: active

Test output:

Running tests...
Test project /home/fancycode/testing/libheif/build
      Start  1: encode
 1/16 Test  #1: encode ....................................   Passed    0.41 sec
      Start  2: extended_type
 2/16 Test  #2: extended_type .............................   Passed    0.73 sec
      Start  3: region
 3/16 Test  #3: region ....................................   Passed    1.39 sec
      Start  4: encode_jpeg2000
 4/16 Test  #4: encode_jpeg2000 ...........................   Passed    0.59 sec
      Start  5: uncompressed_decode
 5/16 Test  #5: uncompressed_decode .......................   Passed    0.33 sec
      Start  6: uncompressed_decode_mono
 6/16 Test  #6: uncompressed_decode_mono ..................   Passed    0.11 sec
      Start  7: uncompressed_decode_rgb
 7/16 Test  #7: uncompressed_decode_rgb ...................   Passed    0.16 sec
      Start  8: uncompressed_decode_rgb16
 8/16 Test  #8: uncompressed_decode_rgb16 .................   Passed    0.11 sec
      Start  9: uncompressed_decode_rgb565
 9/16 Test  #9: uncompressed_decode_rgb565 ................   Passed    0.12 sec
      Start 10: uncompressed_decode_rgb7
10/16 Test #10: uncompressed_decode_rgb7 ..................   Passed    0.13 sec
      Start 11: uncompressed_decode_ycbcr
11/16 Test #11: uncompressed_decode_ycbcr .................   Passed    0.11 sec
      Start 12: uncompressed_decode_ycbcr420
12/16 Test #12: uncompressed_decode_ycbcr420 ..............   Passed    0.11 sec
      Start 13: uncompressed_decode_ycbcr422
13/16 Test #13: uncompressed_decode_ycbcr422 ..............   Passed    0.12 sec
      Start 14: uncompressed_encode
14/16 Test #14: uncompressed_encode .......................   Passed    3.36 sec
      Start 15: uncompressed_decode_generic_compression
15/16 Test #15: uncompressed_decode_generic_compression ...   Passed    0.12 sec
      Start 16: tiffdecode
16/16 Test #16: tiffdecode ................................   Passed    0.03 sec

100% tests passed, 0 tests failed out of 16

Total Test time (real) =   7.97 sec

Let me know if I should run the tests on another platform. Apart from that it looks good to me, so feel free to close this if you want. Thanks!

@fancycode
Copy link
Member Author

@bradh The endianness flags in the uncC header are not taken care of yet. I'll leave this for future work. Currently, it is assuming that all files are stored in big-endian.

Maybe the flags should be checked and files that are not big-endian should be rejected until support for this has been implemented?

@farindk
Copy link
Contributor

farindk commented Oct 29, 2024

Maybe the flags should be checked and files that are not big-endian should be rejected until support for this has been implemented?

This could easily be done here:

static Error uncompressed_image_type_is_supported(const std::shared_ptr<const Box_uncC>& uncC,
const std::shared_ptr<const Box_cmpd>& cmpd)

If I see this correctly, the only case for which libheif will generate little-endian output is for heif_chroma_interleaved_RRGGBB_LE input. I suspect that it will not read back these files correctly.
@bradh : Is this correct, did you test that case?

@fancycode
Copy link
Member Author

This could easily be done here:

There already is a (commented) check:

/*
// TODO: check if this really works... if (uncC->is_components_little_endian()) {
return Error(heif_error_Unsupported_feature,
heif_suberror_Unsupported_data_version,
"Uncompressed components_little_endian == 1 is not implemented yet");
}
*/

@farindk
Copy link
Contributor

farindk commented Oct 29, 2024

Right. Maybe a check could be added whether any component is >8bit so that 8bit still works with any endianness.

@bradh
Copy link
Contributor

bradh commented Oct 29, 2024

It might be safer to check for exactly 8 bits, because uncompressed will do packed in some situations (e.g. 5-6-5).

@farindk
Copy link
Contributor

farindk commented Oct 29, 2024

Added the check here: 4da2842

@farindk farindk closed this as completed Oct 30, 2024
@bradh
Copy link
Contributor

bradh commented Oct 30, 2024

@farindk thank you for the work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants