Skip to content

Fix HEVC test - #1907

Merged
farindk merged 1 commit into
strukturag:masterfrom
scaronni:tests
Sep 21, 2026
Merged

farindk merged 1 commit into
strukturag:masterfrom
scaronni:tests

Conversation

@scaronni

@scaronni scaronni commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

The x265 plugin calls api->encoder_headers() as soon as a sequence encoder is opened, and encode_sequence_frame() calls get_data() immediately afterwards; before any frame is encoded.

That call sees three parameter-set NALs and nothing else, so the optional is still empty when line 260 dereferences it. That is turned into the _M_is_engaged() abort instead of silent UB because of Fedora's default GCC option -Wp,-D_GLIBCXX_ASSERTIONS.

Sample error without this patch:

42/48 Test #20: tai .........................................   Passed    0.19 sec
43/48 Test  #2: encode ......................................   Passed    0.23 sec
44/48 Test #15: sequence_mixed_bit_depth ....................Subprocess aborted**Exception:   0.23 sec
/usr/include/c++/16/optional:1237: constexpr _Tp std::optional<_Tp>::operator->() [with _Tp = Encoder::CodedImageData]: Assertion 'this->_M_is_engaged()' failed.
Randomness seeded to: 312269191

sequence_mixed_bit_depth is a Catch2 v3.7.1 host application.
Run with -? for options

-------------------------------------------------------------------------------
a sequence frame must carry the bit depth of the first frame
  HEVC
-------------------------------------------------------------------------------
/builddir/build/BUILD/libheif-1.23.4-build/libheif-1.23.4/tests/sequence_mixed_bit_depth.cc:206
...............................................................................

/builddir/build/BUILD/libheif-1.23.4-build/libheif-1.23.4/tests/sequence_mixed_bit_depth.cc:206: FAILED:
  {Unknown expression after the reported line}
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases:  1 |  0 passed | 1 failed
assertions: 60 | 59 passed | 1 failed


45/48 Test  #8: item_properties .............................   Passed    0.32 sec
46/48 Test #45: uncompressed_encode .........................   Passed    0.40 sec
47/48 Test #28: omaf ........................................   Passed    0.45 sec
48/48 Test  #3: encode_grid .................................   Passed    0.65 sec

98% tests passed, 1 tests failed out of 48

Total Test time (real) =   0.65 sec

The following tests FAILED:
     15 - sequence_mixed_bit_depth (Subprocess aborted)
Errors while running CTest
error: Bad exit status from /var/tmp/rpm-tmp.lqvMvD (%check)

The x265 plugin calls api->encoder_headers() as soon as a sequence encoder is opened, and encode_sequence_frame() calls get_data() immediately afterwards; before any frame is encoded.

That call sees three parameter-set NALs and nothing else, so the optional is still empty when line 260 dereferences it. That is turned into the _M_is_engaged() abort instead of silent UB because of Fedora's default GCC option -Wp,-D_GLIBCXX_ASSERTIONS.
@rathann

rathann commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

I can confirm this fixes the test in 1.23.4 for me (Fedora rawhide + RPM Fusion).

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Sep 20, 2026
@farindk

farindk commented Sep 21, 2026

Copy link
Copy Markdown
Member

Thanks, confirmed. I reproduced the abort on master with -D_GLIBCXX_ASSERTIONS and the patch fixes it; a two-frame heif-enc -S round trip with the patched build also looks right (hvcC in the sample entry, both frames decode).
VVC doesn't actually hit this path (vvenc doesn't pre-emit headers), but the guard is harmless there. Merging.

@farindk
farindk merged commit 35be0c1 into strukturag:master Sep 21, 2026
41 checks passed
farindk added a commit that referenced this pull request Sep 21, 2026
…e optional

Rename Encoder::encode_sequence_get_data() to encode_sequence_extract_data()
and let it hand out the pending CodedImageData with
std::exchange(m_current_output_data, std::nullopt).

Moving out of a std::optional leaves it engaged with a moved-from value, so
after the first frame the member was never disengaged again. "Nothing
pending" was represented two ways: a disengaged optional before the first
frame, and an engaged one with empty vectors afterwards, which the caller
had to detect with a separate bitstream/properties emptiness check. The
HEVC/VVC get_data() paths were written against the second form only, which
is what let the header-only first drain fixed in #1907 dereference the
disengaged optional.

With the reset, a disengaged optional is the only "no output pending"
state. HEVC and VVC drop the got_some_data flag and use that single check;
HEVC also drops the redundant bitstream.empty() test in front of the hvcC
attachment. AVIF no longer parks an empty CodedImageData when the encoder
is still buffering. AVC keeps its m_output_image_complete gate in front of
the exchange, because x264_encoder_headers() emits an SEI together with the
SPS/PPS and that SEI has to be held back until the slice data arrives. The
JPEG, JPEG 2000 and uncompressed encoders used has_value() as their
encode_sequence_started() answer and now track that with a bool.

Sequence output for HEVC, AVC, AVIF, VVC and JPEG is byte-identical to the
previous code, and the test suite passes with -D_GLIBCXX_ASSERTIONS.
farindk added a commit that referenced this pull request Sep 21, 2026
Export CXXFLAGS with -D_GLIBCXX_ASSERTIONS for the test job in run-ci.sh.
The assertions (container bounds checks, access to a disengaged
std::optional, ...) are independent of NDEBUG, so they also fire in the
Release build that run-ci.sh forces. Distributions like Fedora build with
this flag, which is how the crash fixed in #1907 was found while our own CI
stayed green.

With the assertions enabled, GCC 13 at -O3 reports bogus
-Wstringop-overflow errors ("writing N bytes into a region of size 0" in
stl_algobase.h) for plain std::vector range inserts in several places
(hvcC/avcC/vvcC header getters, test helpers). The same build is
warning-free without the flag and rewriting call sites only moves the
diagnostic around, so that one warning is kept visible but non-fatal in
this job.

Only the test job gets the flag. The other jobs run more compiler versions,
which would each need to be checked for further false positives under
-Werror before enabling it there.
@scaronni
scaronni deleted the tests branch September 21, 2026 06:11
algitbot pushed a commit to alpinelinux/aports that referenced this pull request Sep 21, 2026
bell-sw pushed a commit to bell-sw/alpaquita-aports that referenced this pull request Sep 25, 2026
[ commit 1e9f7dadf6a7ebfd718f3d78fa4ad1c63de0af2b ]

patch from strukturag/libheif#1907
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

Successfully merging this pull request may close these issues.

3 participants