Add a CoreELEC label registry and raw DV/HDR side data for add-ons - #68
Add a CoreELEC label registry and raw DV/HDR side data for add-ons#68matthane wants to merge 12 commits into
Conversation
|
For the battery item: I will than drop the old commit completely afterwards. |
a11d541 to
5169d46
Compare
|
Done. The old patch is now fully reverted in its own commit and the Linux version comes back separately on a CE id. The revert also restores the kernel suffix on the OS version line and drops the AMLogic title text since those came from the same patch. |
|
While this is open, I've been experimenting on top of this branch with even more DV labels (L2/L8 trim passes for example) and HDR10+ dynamic metadata labels. With those added AMLFrameMetadata.h grows past 2000 lines, so I think it makes sense to create more of a formal framework per format. AMLFrameMetadata.h keeping the registry and shared plumbing, with the DV and HDR10+ parsing/rendering moving to their own headers (AMLDoviMetadata.h, AMLHdr10PlusMetadata.h). Then, we would have a cleaner structure that I think would be easier for any future additions too. Should I add that work and update this PR with it? Or is it easier to keep this one as is, and send a follow up with those changes pending this one? |
|
I am still offline so can't check much. I would prefer to transfer the raw HDR meta data to a add-on as byte array. So all parsing and structs can be moved outside. This is much easier to maintain as when in main core code. CE kernel just do the same. So pass the raw data to a buffer of the addon would be best. |
|
Ah ok, yeah I see what you mean. So then it would be passing the raw bytes (base64) in something like player.process(video.dovi.rpu) and player.process(video.hdr10plus.sei). And then an addon could parse them and open up the specific metadata as fields and be used by other addons as a dependency. That would definitely clean things in the core base! I'll rework this PR to strip the parsing and structs down to just the raw pass-through. |
|
Yes, or just raw data with the info what side data it includes. So the raw data can be typeless transfered to the add-on. |
|
Ok, I'll experiment and see what works with minimal core changes. TinyPPI currently uses a binary (hdrprobe) to oneshot inspect each file on playback and get the hdr/dv metadata from it's output json. But, it's all static metadata because of this. |
|
I think we can live with it if the bitstream converter provides the current frame data through an interface, as this would require minimal code changes. However, I’m not sure how this will work when the FFmpeg BSF parser is used. Would this be done by extracting the side data? |
|
Basically, anything FFmpeg already delivers as side data (the HDR10+ block additions from MKV, the DV config record) is just passed through. For the in-band payloads (RPU NAL, SEIs) the codec grabs the bytes from the raw demux packet before any conversion runs, so it doesn't matter whether the bitstream converter or an FFmpeg BSF processes the packet afterwards. Nothing needs an interface on the converter at all. It all comes out as one label with the payloads tagged and base64'd, and the add-on does the rest. I think it will be easier to discuss or review further once I've reshaped the PR to simplify it accordingly. Thanks for your feedback so far, I think it will be much more elegant this way. |
34c5ba8 to
a5ec1a9
Compare
b10760a to
a7061ef
Compare
matthane
changed the title
|
Ok, I've updated the title, description, and commits now. The biggest chunk is b85b834 because it holds how we gather the raw metadata from the packets, before the bitstream converter can strip or rewrite it, and package it for the new label. Edit: While adapting to the new base I found the HDR10+ kernel feed broke with the ffmpeg 9.0 bump. The old raw side data patch was dropped in favor of AV_PKT_DATA_DYNAMIC_HDR_SMPTE_2094_APP5, but that type carries SMPTE 2094-50, which is a different format. Real HDR10+ mkv block additions now arrive parsed as AVDynamicHDRPlus under AV_PKT_DATA_DYNAMIC_HDR10_PLUS, so the checkin never fires. The new first commit 768c92a restores the feed by querying the right type and rebuilding the raw T.35 with ffmpeg's own serializer, the same way the ffmpeg mkv muxer writes it. |
a7061ef to
8673e43
Compare
There was a problem hiding this comment.
Is it ok for you when I squash this fix to c4b6ceb ?
I tested it and HDR10p pop up again on my VP9 sample.
And VP9 with HDR10p is also handled correct?
I see only h265 and AV1 handling. Maybe make it more "codec independent"?
|
Sure, go ahead and squash it, that's the cleanest place for it. And good catch on VP9! I always forget about it. I've removed that gate so it's fully codec independent, and verified with a VP9 HDR10+ sample that the label now carries its per-frame payloads too. Will push together with the rebase once you've squashed. |
a5ec1a9 to
26749d0
Compare
8673e43 to
590d3ed
Compare
There was a problem hiding this comment.
Instead "move" please revert complete Add Amlogic display mode, EOFT and colourimetry and alsa: provide ALSA output channel layout and make a new implemention.
Otherwise it will continue fail on rebase as it's still be applied to core code and then removed again.
Once merged I will drop whole commit and revert on a rebase afterwards.
| registry.Add("player.process(amlogic.eoft_gamut)", CE_PLAYER_PROCESS_AML_EOFT_GAMUT); | ||
| registry.Add("player.process(audiochannelssink)", CE_PLAYER_PROCESS_AUDIOCHANNELS_SINK); | ||
| registry.Add("system.linuxver", CE_SYSTEM_LINUX_VER); | ||
| registry.Add("player.process(video.sidedata)", CE_PLAYER_PROCESS_VIDEO_SIDEDATA); |
There was a problem hiding this comment.
CE_PLAYER_PROCESS_VIDEO_SIDEDATA is not defined?
There was a problem hiding this comment.
Done, moved to CEGUIInfoRegistry.h next to the others.
| std::map<double, AMLFrameMetadata> m_queue; | ||
| }; | ||
|
|
||
| constexpr uint32_t CE_PLAYER_PROCESS_VIDEO_SIDEDATA = PLAYER_PROCESS_START + 34; |
There was a problem hiding this comment.
CE_PLAYER_PROCESS_VIDEO_SIDEDATA is definded here?
It should rest at the same place as the others.
There was a problem hiding this comment.
Done, moved to CEGUIInfoRegistry.h next to the others.
| if (payload > end - p) | ||
| break; | ||
|
|
||
| if (type == 4) // user_data_registered_itu_t_t35 |
There was a problem hiding this comment.
Please use enum, preferred if already be defined in source.
Easier to read in future. Otherwise each type must be searched again when checking the source.
Same with the NAL units, 39, 62,...
There was a problem hiding this comment.
Done. The HEVC NAL and SEI payload types are enums now. As far as I can tell ffmpeg doesn't install sei.h or hevc.h, so those are defined locally, and the AV1 OBU types come from obu_util.h the same way AMLCodec already uses it.
| // latch from the original demuxer payload, before Convert() can strip or | ||
| // rewrite it. Dual-track streams are latched from the base layer at pair | ||
| // completion: the EL carries its own static SEIs with different values | ||
| if (m_hints.codec == AV_CODEC_ID_HEVC && !packet.isDualStream) |
There was a problem hiding this comment.
Better use m_hints.hdrType to check what side data is handled.
This can avoid missing codec id in the switch.
There was a problem hiding this comment.
Reworked to check hdrType, with a codec switch picking the parser and a default arm so a new codec can't slip through unhandled. I went with hdrType != NONE rather than matching specific types. From what I can see DetermineHdrType never returns HDR10PLUS, the plus only gets discovered from the bitstream and reported back through process info, so HDR10+ content seems to reach the codec still typed as HDR10. DV files whose container lacks the config record (older mkv muxes, TS without the descriptor) also appear to type as HDR10 while still carrying RPUs.
| { | ||
| m_metaLeadLogged = true; | ||
| CLog::Log(LOGDEBUG, "{}: frame metadata pts lead {:.0f} ms", __MODULE_NAME__, | ||
| (m_lastCommitPts - target) / 1000.0); |
There was a problem hiding this comment.
Please follow code style like: (renderPts - nextFramePts) / DVD_TIME_BASE
26749d0 to
6862e6d
Compare
CE features register label names and info providers from CE owned files instead of adding rows to the upstream tables. Labels resolve by name, so CE is no longer bounded by the free id slots in Kodi's info label blocks. Providers append at the back of the provider list so upstream answers first.
This reverts commit fd8e4ef.
Readd the Linux version row on a CE id now that the battery id matches upstream again.
The reverted patch also restyled the System Info screen. Those parts come back here on their own so they can be kept or dropped independently: the AMLogic header line, the hidden BogoMips and Revision rows, the bracketed os version, and the kernel suffix dropped from osVersionInfo now that the Linux version has its own row.
This reverts commit b2e4ec8.
This reverts commit 7cdedfe.
Readd the amlogic.pixformat, amlogic.displaymode and amlogic.eoft_gamut labels on CE ids, answered by the CE provider instead of upstream table rows and cases. The check script guards the CE id ranges against upstream growth on future rebases.
Readd the audiochannelssink label on a CE id with the sink side plumbing that feeds it. The ProcessInfo getter and its debug log do not come back, the label reads the data cache directly.
The reverted patch also rounded the videofps label to a trimmed fractional form. That presentation of an upstream value stays.
Locates the DV RPU, HDR10+ SEI and HDR static payloads in the stream and publishes them base64 encoded under one JSON label, so add-ons parse the raw bytes and core carries no metadata formats. Values are sequenced per pts and released when the render clock reaches their frame.
590d3ed to
bcec4a9
Compare
Done. Both commits are fully reverted in their own commits now and the labels come back as new implementations on the registry, same pattern as the kernel version one. From what I could see the ProcessInfo getter and its debug log had no other users, so I left them out of the re-add, but I can put them back if you'd rather keep them. Once you drop the originals on a rebase the reverts can go with them. |
2 participants
Description
This adds a small label registry so that CoreELEC info labels can live in CE owned files instead of rows in the Kodi label tables, and moves the existing CE labels onto it. On top of that, the Amlogic codec publishes the stream's raw Dolby Vision and HDR metadata under one new label, Player.Process(video.sidedata): the DV RPU, HDR10+ SEI, static HDR SEIs and DV configuration record, latched from the demuxer packets before any conversion can strip them, base64 encoded in one small JSON object, and sequenced per pts against the render clock so a poll tracks the frame on screen. The latch is codec agnostic and takes both the payloads carried in band and the ones the container hands over as side data. Core parses none of it. The byte contract is documented in the label's dox entry.
Motivation and context
The goal is to lower the maintenance cost of CE labels, not add to it. The shared Kodi files carry two small hooks, everything else lives in CE owned files, labels resolve by name instead of competing for id slots, and a rebase check script guards the CE ranges. Any future side data type is just a new key in the JSON object.
Parsing lives outside the tree in a module add-on that binds libdovi and the platform's libavutil directly: https://github.com/matthane/script.module.sidedata. Ideally, this script module could live in a CE repo for addons to easily use as a dependency.
System.BatteryLevel moves back to its upstream id with linuxver served from a CE id instead, and the System Info presentation from the old patch is re-applied in its own commit so it can be kept or dropped independently.
How has this been tested?
On a Ugoos AM9 Pro (S905X5-J) running a CoreELEC 22 test build. Playback of all current supported DV profiles, including profile 10 AV1, with the parsed output cross-checked byte for byte against dovi_tool and FFmpeg on real titles, shown by a diagnostic overlay: https://github.com/matthane/script.dvhdr.labels.diagnostic/releases/tag/v3.0.3 HDR10+ arriving as container side data was checked the same way.
What is the effect on users?
Add-ons can read the stream's live Dolby Vision and HDR metadata during playback. There should be no visible change otherwise.
Screenshots (if appropriate):
Types of change
Checklist: