Crash when encoding AVIF using rav1e

[novomesk]

Hello,

I am experiencing crashes when Windows GIMP tries to export AVIF images with transparency. It can be reproduced using heif-enc.exe under MSYS2 on Windows but for some reason it is not occurring on my Linux.

More information in the following issues:

msys2/MINGW-packages#9869

xiph/rav1e#2836

[farindk]

Could you please confirm that the crash disappears when these two lines

  rav1e_frame_fill_plane(rav1eFrame.get(), 1, Cb, strideCb * uvHeight, strideCb, byteWidth);
  rav1e_frame_fill_plane(rav1eFrame.get(), 2, Cr, strideCr * uvHeight, strideCr, byteWidth);

are removed from heif_encoder_rav1e.cc ?

[novomesk]

Removing the two lines and -Werror avoids the crash.

It also makes the image grayscale-looking.

[farindk]

It also makes the image grayscale-looking.

Sure. This is just a test to see if there crash goes away. Problem is that the alpha input image is greyscale and the Cb/Cr pointers point to nothing.

[farindk]

I'm currently preparing the release v1.14.0 and was wondering if this issue is still relevant.
I went through the code with the testfile.zip you provided, but cannot reproduce the crash (I'm on Linux and know that it doesn't crash there, but I was checking for null pointers).
The strange thing is that I also cannot see anymore how this can happen even in v1.12.0.

The crash looks like the input alpha image is monochrome only. But as far as I can see, this cannot happen since rav1e_query_input_colorspace2() forces a conversion to either 4:2:0, 4:2:2, or 4:4:4 here:

libheif/libheif/heif_context.cc

Line 2209 in 7fc9427

src_image = convert_colorspace(image, colorspace, chroma, nclx_profile);

If you still have these crashes, could you please check the value of 'chroma' in rav1e_encode_image() before the rav1e_frame_fill_plane() calls crash?

[novomesk]

Please give me few days. I'll install MSYS2 environment again and try to reproduce the problem after building the libheif there.

[farindk]

Sure, after that long time, a few days won't matter.

[kleisauke]

I could still reproduce this crash on the master branch with the vips and heif-enc utility. Applying this patch:

--- a/libheif/heif_encoder_rav1e.cc
+++ b/libheif/heif_encoder_rav1e.cc
@@ -512,6 +512,7 @@ struct heif_error rav1e_encode_image(void* encoder_raw, const struct heif_image*
 
 
   const heif_chroma chroma = heif_image_get_chroma_format(image);
+  std::cout << "chroma=" << chroma << std::endl;
 
   uint8_t yShift = 0;
   RaChromaSampling chromaSampling;

Prints chroma=1 (heif_chroma_420) with the provided test image, just before exiting with -1073741819 (access violation exception) on Windows.

libheif/libheif/heif.h

Line 903 in 6d15810

heif_chroma_420 = 1,

Applying this patch:

--- a/libheif/heif_encoder_rav1e.cc
+++ b/libheif/heif_encoder_rav1e.cc
@@ -522,6 +522,7 @@ struct heif_error rav1e_encode_image(void* encoder_raw, const struct heif_image*
   if (input_class == heif_image_input_class_alpha) {
     chromaSampling = RA_CHROMA_SAMPLING_CS420; // I can't seem to get RA_CHROMA_SAMPLING_CS400 to work right now, unfortunately
     chromaPosition = RA_CHROMA_SAMPLE_POSITION_UNKNOWN; // TODO: set to CENTER when AV1 and rav1e supports this
+    yShift = 1;
   }
   else {
     switch (chroma) {

Seems to fix this, and the output image looks OK. So, perhaps this whole if-statement:

libheif/libheif/heif_encoder_rav1e.cc

Lines 521 to 525 in 5fdba25

	if (input_class == heif_image_input_class_alpha) {
	chromaSampling = RA_CHROMA_SAMPLING_CS420; // I can't seem to get RA_CHROMA_SAMPLING_CS400 to work right now, unfortunately
	chromaPosition = RA_CHROMA_SAMPLE_POSITION_UNKNOWN; // TODO: set to CENTER when AV1 and rav1e supports this
	}
	else {

Can be removed, assuming it always falls into this case statement for (monochrome-)alpha images:

libheif/libheif/heif_encoder_rav1e.cc

Lines 535 to 539 in 5fdba25

	case heif_chroma_420:
	chromaSampling = RA_CHROMA_SAMPLING_CS420;
	chromaPosition = RA_CHROMA_SAMPLE_POSITION_UNKNOWN; // TODO: set to CENTER when AV1 and rav1e supports this
	yShift = 1;
	break;

BTW, I couldn't test commit 6d15810 (and commits after that) in combination with libvips, since the plugins are no longer implicitly registered at startup. Do we have to call heif_init() in libraries that depend on libheif?

[farindk]

@kleisauke Thanks, that probably is the issue. I was always looking for a null chroma pointer, but in fact it seems to be the wrong yShift value.

Concerning the change with heif_init(): even after the introduction of heif_init(), the plugins should be initialized by default. This works for me (Linux), but maybe there is also a behavioral difference on Windows? Maybe because the execution order of static class initialization at startup is undefined?

I've added a separate issue for this (#668).

[novomesk]

I am able to reproduce the crash on MSYS2:

#0  0x00007ffa79e644c4 in msvcrt!memmove ()
   from C:\WINDOWS\System32\msvcrt.dll
#1  0x00007ffa56ca5cfb in ?? () from C:\msys64\mingw64\bin\rav1e.dll
#2  0x00007ffa56c3d168 in ?? () from C:\msys64\mingw64\bin\rav1e.dll
#3  0x00007ff6ca52ba5d in rav1e_encode_image (encoder_raw=0x182f57548f0,
    image=<optimized out>, input_class=<optimized out>)
    at heif_encoder_rav1e.cc:646
#4  0x00007ff6ca515b29 in heif::HeifContext::encode_image_as_av1 (
    this=this@entry=0x182f55e3c30,
    image=std::shared_ptr<heif::HeifPixelImage> (use count 1, weak count 1) = {...}, encoder=encoder@entry=0x182f57546f0,
    options=options@entry=0x1b947ff980,
    input_class=input_class@entry=heif_image_input_class_alpha,
    out_image=std::shared_ptr<heif::HeifContext::Image> (use count 2, weak count 0) = {...}) at heif_context.cc:2274

It was build using autotools.

BTW, when I try to build using cmake, all heif utilities crash immediately:

#0  0x00007ffa58c5597a in ?? () from C:\msys64\mingw64\bin\libstdc++-6.dll
#1  0x00007ff780e8d85b in std::_Rb_tree_iterator<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> > >::operator-- (
    this=0x9cc29ff650)
    at C:/msys64/mingw64/include/c++/12.2.0/bits/stl_tree.h:302
#2  0x00007ff780eab409 in std::_Rb_tree<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> >, std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> >, std::_Identity<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> > >, heif::encoder_descriptor_priority_order, std::allocator<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> > > >::_M_get_insert_unique_pos (this=0x7ff780efd1a0 <heif::s_encoder_descriptors>,
    __k=std::unique_ptr<heif_encoder_descriptor> = {...})
    at C:/msys64/mingw64/include/c++/12.2.0/bits/stl_tree.h:2126
#3  0x00007ff780eab1ea in std::_Rb_tree<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> >, std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> >, std::_Identity<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> > >, heif::encoder_descriptor_priority_order, std::allocator<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> > > >::_M_insert_unique<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> > > (
    this=0x7ff780efd1a0 <heif::s_encoder_descriptors>, __v=...)
    at C:/msys64/mingw64/include/c++/12.2.0/bits/stl_tree.h:2170
#4  0x00007ff780e98f5c in std::set<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> >, heif::encoder_descriptor_priority_order, std::allocator<std::unique_ptr<heif_encoder_descriptor, std::default_delete<heif_encoder_descriptor> > > >::insert (
    this=0x7ff780efd1a0 <heif::s_encoder_descriptors>, __x=...)
    at C:/msys64/mingw64/include/c++/12.2.0/bits/stl_set.h:521
#5  0x00007ff780e31e2d in heif::register_encoder (
    encoder_plugin=0x7ff780ec2aa0 <encoder_plugin_aom>)
    at C:/msys64/home/daniel/libheif/libheif/heif_plugin_registry.cc:126
#6  0x00007ff780e31cb5 in heif::register_default_plugins ()
    at C:/msys64/home/daniel/libheif/libheif/heif_plugin_registry.cc:74
#7  0x00007ff780e567a1 in Register_Default_Plugins::Register_Default_Plugins (
    this=0x7ff780efd1e4 <dummy>)
    at C:/msys64/home/daniel/libheif/libheif/heif_init.cc:85
#8  0x00007ff780e330cd in __static_initialization_and_destruction_0 (
    __initialize_p=1, __priority=65535)
    at C:/msys64/home/daniel/libheif/libheif/heif_init.cc:87
#9  0x00007ff780e330eb in _GLOBAL__sub_I_heif_init ()
    at C:/msys64/home/daniel/libheif/libheif/heif_init.cc:87
#10 0x00007ff780e49332 in __do_global_ctors ()
    at C:/M/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/gccmain.c:44
#11 0x00007ff780e4938f in __main ()
    at C:/M/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/gccmain.c:58
#12 0x00007ff780e01388 in __tmainCRTStartup ()
    at C:/M/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crtexe.c:321
#13 0x00007ff780e014e6 in mainCRTStartup ()
    at C:/M/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crtexe.c:206

[farindk]

@novomesk This is the #668 issue. It seems that the Register_Default_Plugins static object is initialized before the std::set in heif_plugin_registry.cc. I'll switch that back.

[farindk]

@novomesk Please check again. I added fixes for #554 and #668.

[novomesk]

Thanks, both are working now.

[kleisauke]

Commit 0020008 seems to fix this for me on Windows (tested in combination with commit libvips/build-win64-mxe@846d74f). Though, I only checked it with the provided sample image.

(fwiw, libvips still prefers aom over the dav1d+rav1e combo due to build difficulties and binary size, but perhaps we'll revisit this)