Skip to content

AML: serialise the DRM objects against the display rebuild - #90

Open
allolive wants to merge 394 commits into
CoreELEC:aml-5.15.196-22.0from
allolive:pr-aml-display-rebuild
Open

allolive wants to merge 394 commits into
CoreELEC:aml-5.15.196-22.0from
allolive:pr-aml-display-rebuild

Conversation

@allolive

@allolive allolive commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Description

A few small patches for things that look like they could go wrong: values that can be read before they are set, copies and retries without a bound, and objects read from one thread while another rebuilds them.

Nothing here came from a reported failure. They are split into a few small pull requests so that one patch that is not wanted/needs discussion does not hold up the others.

Motivation and context

Reading the code, these look like they could bite. None of them has been seen to.

How has this been tested?

Regular use looks stable on an Amlogic S922X-J (g12b) box on CoreELEC 22 - playback, seeking, mode switches, Dolby Vision and non-Dolby-Vision content, decoder open and close. Nothing has hit the failure paths themselves.

What is the effect on users?

Maybe stability improvements.

Screenshots (if appropriate):

N/A

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (v22 new feature polish) (non-breaking change which improves a new feature in v22)
  • Improvement (other) (non-breaking change which improves other 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 does not touch code)
  • Student submission (PR was done for educational purposes and will be reviewed with support)
  • 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

Portisch and others added 30 commits September 8, 2026 09:18
The check by ifstream do fail on files with write only permission.
Used since Amlogic kernel 4.9.180
Use simple mixer control to set codec format for SPDIF-A and SPDIF-B.
Also set SPDIF to HDMITX by simple mixer control to enable correct SPDIF output.
Since kernel 4.9.180 di_multi is used on new platforms.
VC1 progressive does need to be deinterlaced as otherwise it get played too slow.
vadj1 need to be enabled to be able to use brightness1 and contrast1 controls.
brightness1 control range got extended to -255..255
Fixed refresh rate switching for non-standard 4K and more higher 5K, 8K resolutions.
Fixed correct switching while playing video file in bigger resolution and opening video file with lower resolution.
Fixed using double refresh rate with whitelist.
On Kodi start GetCPUInfo is not initialized.
Regex expression needs to be adjusted because of the use of SysfsPath
…ting

Update methods to match current kernel.
Kodi Nexus do not stop the video thread when a media file get opened
while another one is already playing. So we must close the currently
open decoder before opening a new one as the kernel is able to handle
multiple decoders in parallel but Kodi is not.
Remove not needed delays.
Check correct free_len when adding new chunk to hardware decoder input buffer.
'DequeueBuffer': do return EAGAIN when no new frame is decoded.
But this does not mean the frame buffer is empty at all.
Reset the decoder when no new frame is available within the new timeout, default is 5s.
'GetPicture': signal VC_NONE if DequeueBuffer failed and hardware buffer is not empty
av_grow_packet() allocates a dynamic buffer which must be freed when we
close the decoder
The <EGL/fbdev_window.h> include was missing and native window was NULL
as _FBDEV_WINDOW_H_ was not defined.
If 'dovi.ko' is available enable the DV option.
If the mode switch is really required is checked afterwards in 
WinSystemAmlogic system.
Portisch and others added 26 commits September 8, 2026 09:19
When a stream starts, the hardware decoder buffer is initially
empty, so no new frame is available for decoding. The stall
detection logic did not account for this state and kept
retrying indefinitely, causing an endless loop.
Keep the udev monitor active across hotplug events and defer HotplugEvent() processing to the window-system message pump. This avoids stopping the monitor from its callback and keeps DRM hotplug handling on Kodi's main thread.
The lazy read in IsHDRDisplay never cleared old flags, so a sink swap
could keep the previous display's capabilities, and the codec could
query the cache before anything filled it. Read hdr_cap and dv_cap at
init and on hotplug instead, rebuilding the object each time.
The else got lost in an earlier rework, so the warning fired on every
hotplug.
First frame is only used to configure the render.
Visibility was decided once at boot and never revisited, so a sink
change could leave the display settings wrongly shown or hidden for the
session. Re-evaluate it on display events from the live display verdict.
The settings chained by enable dependencies stay device-keyed so a
disabled row always has its cause on screen.
The statics latched the display verdict at the first call, so a sink
swap left the demuxer's dual-layer handling frozen on the old display
while the decoder followed the live verdict.
Force the Player-Led policy to have DV converted to the highest possible 
HDR format supported by the connected TV.
m_nalLengthSize was only read from hvcC, so it stayed 0 for H.264 and every
caller taking the length-prefixed path fell through to Annex-B start-code
scanning on avcC packets, finding no NAL units at all.

Read lengthSizeMinusOne from avcC byte 4, before the extradata is rewritten to
Annex-B.

This close CoreELEC#75.
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")
The 1000/1001 modes divide by 1.001f, which lands one ULP below the rate the
player derives from the same ratio in double:

    (float)(24/1.001f)   23.976022720336914
    (float)(24/1.001)    23.976024627685547

So == is false and CheckEnableClockSync()'s perfect match test cannot fire.
Dividing in double makes the two match bit for bit. Integer modes never divide.

The widening is written out because the build is -Werror=double-promotion.

Signed-off-by: Olivier Allauzen <olivier.allauzen@gmail.com>
GetPicture derives each frame's duration from the gap to the previous
timestamp; a repeated or backwards PTS produced a nonsense duration.
Fall back to the stream's nominal frame duration when the timestamp
does not advance.

GetPicture()'s fallback duration computed video_rate * DVD_TIME_BASE in
unsigned int. With DVD_TIME_BASE = 1000000 that overflows once video_rate goes
above 4294, i.e. content below ~22.35 fps, before the widening to double - a
wrong duration on low-fps sources. Widen video_rate first.

Signed-off-by: allolive <160342668+allolive@users.noreply.github.com>
CAMLDRMUtils publishes m_connector, m_encoder, m_crtc and m_plane and rebuilds them
on hotplug, on the app thread. The video thread reads them at the same time -
CAMLCodec writes its Dolby Vision properties at decoder open and reads them back at
close, both through m_crtc->crtc_id.

The connected flag does not stand in for a lock. CleanAndClose frees the objects
and only then marks the device disconnected, so a reader can pass the check and
dereference freed memory. The rebuild marks it connected as soon as it has a
connector, then does the plane, encoder and crtc lookups with m_crtc still null.

Serialise the accessors and the rebuild with one recursive lock - recursive because
three of them call each other. The render path is left alone: it runs on the same
thread as the rebuild.

Signed-off-by: allolive <160342668+allolive@users.noreply.github.com>
aml_init_drmDevice() throws on any of seven failures, having called CleanAndClose()
first. Nothing between it and the frame loop catches, so a failed rebuild takes the
process down.

Catch it where the hotplug is handled and leave presentation off. What is left has
no connector, so the accessors take their not-connected branch and read
m_connector - guard that read too.

Nothing is retried here: a later hotplug runs the rebuild again, but until one
arrives the display stays dark. This depends on the previous change serialising
those reads.

Signed-off-by: allolive <160342668+allolive@users.noreply.github.com>
@Portisch
Portisch force-pushed the aml-5.15.196-22.0 branch 4 times, most recently from 6c4347c to 23448cd Compare September 14, 2026 18:48
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.