Skip to content

VideoPlayer: check demux stream before dual layer test - #80

Merged
Portisch merged 1 commit into
CoreELEC:aml-5.15.196-22.0from
shaneomac1337:fix-dvd-menu-dual-layer-null
Aug 30, 2026
Merged

VideoPlayer: check demux stream before dual layer test#80
Portisch merged 1 commit into
CoreELEC:aml-5.15.196-22.0from
shaneomac1337:fix-dvd-menu-dual-layer-null

Conversation

@shaneomac1337

Copy link
Copy Markdown
Contributor

Description

CVideoPlayer::OpenDefaultStreams() dereferences what m_pDemuxer->GetStream() returns without checking it:

if (m_pDemuxer)
{
  CDemuxStreamVideo* vstream = static_cast<CDemuxStreamVideo*>(m_pDemuxer->GetStream(stream.demuxerId, stream.id));
  if (vstream->isDualStream && vstream->isELStream)
    continue;
}

The guard checks m_pDemuxer, but not the stream it hands back. That loop walks m_SelectionStreams, which also holds NAV, Bluray, text and external streams, and their id is not a demuxer uniqueIdCSelectionStreams::Update() sets s.id = stream->uniqueId for demuxer streams, but for DVD navigator streams it sets s.id = i, a libdvdnav index. In the DVD menu domain the demuxer has no streams at all yet, so the lookup returns nullptr and the next instruction faults.

The cast is unchecked for type too, which I only noticed while fixing the first part. CDVDDemux::GetStream(int64_t, int) throws the demuxer id away (return GetStream(iStreamId);) and CDVDDemuxFFmpeg::m_streams is one map keyed by uniqueId across all stream types, so a NAV video stream with id = 1 gets back whatever holds uniqueId 1 — on a DVD that is usually an audio stream. Reading isDualStream/isELStream off a CDemuxStreamAudio reads past the end of the object and can quietly skip a valid video stream. Not fatal, so it would have survived a plain null check.

So I gated on the source mask and validated the stream before casting, which is what IsValidStream() already does a few hundred lines further down. CDVDDemuxFFmpeg::ReadInternal() guards this same cast the same way where it latches these two flags onto the packet — null check, then stream->type == StreamType::VIDEO, then cast.

Motivation and context

Every nightly since 2026-08-20 segfaults on Amlogic when a DVD is opened from its menu (VIDEO_TS.IFO or ISO). Playing VTS_*.VOB directly still works, which is why it first looked like certain discs were broken rather than DVD playback in general.

It is a regression from 2a9a87e ("Videoplayer: open base layer as default on dual layer video"). What that commit is for still works after this change: isDualStream/isELStream only ever get set in CDVDDemuxFFmpeg on the Dolby Vision dual layer path behind aml_dolby_vision_enabled(), so they can only exist on STREAM_SOURCE_DEMUX streams. A stream the demuxer does not have cannot be an enhancement layer, so restricting the probe to demuxer streams cannot stop the EL being excluded where it actually matters. Bluray goes through the else if (demuxer) branch in Update() and keeps source STREAM_SOURCE_DEMUX, so DV P7 on Bluray is unaffected.

How has this been tested?

I have not compile-tested this — I do not have a build tree set up. It touches one block, adds no includes, and only uses things the file already has (STREAM_SOURCE_MASK(stream.source) == STREAM_SOURCE_DEMUX is the same expression as line 1230, and CDemuxStream* is used bare at 1459, 1468 and 1486), but that is me reading the code, not a build. It applies cleanly to aml-5.15.196-22.0 at 9f5601b.

What I did test is the diagnosis, on a Ugoos AM9 Pro (S905X5) on 22.0-Piers_nightly_20260828, Kodi 0f9a78be:

Five crashes, same backtrace every time — CVideoPlayer::OpenDefaultStreams(bool)Prepare()Process(), always PC 0x9dbf00. The kernel fault dump gives x0 : 0000000000000000 in all five, ESR 0x92000006 (data abort, level 2 translation fault, read), faulting address 0x185. The mapping is 00400000-01fd1000 r-xp 00000000, so the binary is not PIE and the PC is a link time address I could look up directly:

0x9dbefc <OpenDefaultStreams+652>:  blr   x3                 ; devirtualised GetStream()
0x9dbf00 <OpenDefaultStreams+656>:  ldrb  w1, [x0, #389]     ; isDualStream  <-- SIGSEGV
0x9dbf04 <OpenDefaultStreams+660>:  tbz   w1, #0, +672
0x9dbf08 <OpenDefaultStreams+664>:  ldrb  w0, [x0, #390]     ; isELStream

0x185 is 389, so the fault address is x0 + offsetof(isDualStream) with x0 null. Nothing between the call and the load. GCC devirtualising GetStream() down to GetStream(int) is also what makes the type confusion above reachable.

To be sure it was this and not something nearby, I patched the ldrb at 0x9dbf00 in the shipped kodi.bin into a branch past the check — the old control flow, before 2a9a87e — and bind mounted it over /usr/lib/kodi/kodi.bin. The discs that crashed five times now play through their menus, and the log shows Opening stream: 1 source: 512 where it used to just stop. That confirms the cause and the code path, but it is not this patch's source, which is still uncompiled.

For comparison, nightly_20260730 (Kodi 726799f4, before the commit) logs Opening stream: 1 source: 512 on the same discs at exactly the point where 0828 dies — source: 512 being STREAM_SOURCE_NAV, id 1 the nav index. I hit it on three different discs, over both a CIFS mount and smb://, so it is not disc or path specific.

What is the effect on users?

DVD menus work again on Amlogic. Dolby Vision profile 7 is unchanged — the enhancement layer is still kept out of default video stream selection on the demuxer streams that can actually carry those flags.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

OpenDefaultStreams dereferences the result of m_pDemuxer->GetStream()
without a null check. On the DVD menu domain the demuxer holds no
streams, while the selection streams come from libdvdnav with source
STREAM_SOURCE_NAV and a nav index as id, so the lookup returns nullptr
and Kodi segfaults before the first stream is opened.

Only demuxer streams can carry the dual layer flags, so restrict the
probe to STREAM_SOURCE_DEMUX and verify the stream exists and really is
a video stream before casting. CDVDDemux::GetStream() ignores the
demuxer id and looks up by stream id alone across all stream types, so
the cast was unchecked for type as well.

Fixes: 2a9a87e ("Videoplayer: open base layer as default on dual layer video")
@Portisch
Portisch merged commit ddc9563 into CoreELEC:aml-5.15.196-22.0 Aug 30, 2026
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.

2 participants