Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[depends][Android] Add base dependencies setup for libdovi #22546

Merged
merged 3 commits into from Sep 3, 2023

Conversation

quietvoid
Copy link
Contributor

@quietvoid quietvoid commented Jan 21, 2023

Description

The changes bring support for the Rust toolchain with the goal of being able to integrate C compatible dependencies through cargo-c.
This PR also adds the optional libdovi dependency for Android builds.

Motivation and context

The main motivation for this is to be able to convert Dolby Vision on-the-fly, so that Android devices have less issues.
This requires libdovi (which is a Rust/C compatible library).

There are also other usecases for Rust such as binary addons, which seem to have been previously proposed GSoC projects.

How has this been tested?

Tested successful build for all Android target platforms.
Runtime tested on FireTV Stick 4K (2018) and 4K Max (2021), so armv7.

What is the effect on users?

None by default, as the added dependencies are built optionally with a configure flag.
Only the build process is affected, it's still unknown to me how the new dependencies behave on all supported platforms.
As the dependencies are limited to Android only, I don't think it can have a negative effect.

Testing thread for Android: https://forum.kodi.tv/showthread.php?tid=371557

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)
  • 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

@quietvoid quietvoid changed the title [Android] Add base dependencies setup for libdovi [depends][Android] Add base dependencies setup for libdovi Jan 21, 2023
@quietvoid
Copy link
Contributor Author

quietvoid commented Jan 21, 2023

Opened this as draft as there are very likely changes needed.
Feedback welcome.

@fuzzard
Copy link
Contributor

fuzzard commented Jan 21, 2023

I would say you are going to have to implement something in the core code that uses this before it's looked at.

Will be interesting to see how much the depends build times increase with rustup

@fritsch
Copy link
Member

fritsch commented Jan 21, 2023

Maybe - as the stuff is used from the sidedata (when used from mkv container) at the end - a place inside ffmpeg would be great I think, though having it as a customer converter, like e.g. VideoPlayerPassthroughCodec would be great for the start as well - here it could be used as a transcoder, similar to what we do with our Dolby Transcoding.

@fuzzard
Copy link
Contributor

fuzzard commented Jan 21, 2023

You'll want to add a "temp" commit that actually enables these flags for this PR so we can actually have them built as well

@fritsch
Copy link
Member

fritsch commented Jan 21, 2023

I like that we have a PR here in the first place. Which could stay open or at a point also could point to a dovi public branch in our repo, so that other contributors can help on this basis, so that a future solution that nicely fits into kodi can be found? Chances are higher, when dependencies are already available I'd say.

@fuzzard
Copy link
Contributor

fuzzard commented Jan 21, 2023

Yeah, but we aren't ever going to merge a dependency not actively used, and also this doesn't even test the build of it, as default is off.

@quietvoid
Copy link
Contributor Author

quietvoid commented Jan 21, 2023

I could add a simple commit that does nothing but use the libdovi dependency, I guess.
But I don't know how useful that would be as the configure flag still needs to be enabled.
edit: oh, right a temp flag.

@quietvoid
Copy link
Contributor Author

quietvoid commented Jan 21, 2023

I've made the dependency default for Android.
It'll most likely fail x86 targets as HOST won't match the Rust target platforms.
It'll likely need a new variable specifically for Rust targets.

Oh, actually the targets only differ for non-Android, so it should be fine.

@quietvoid
Copy link
Contributor Author

quietvoid commented Jan 22, 2023

This is the commit I'd actually use: quietvoid@84208b7
I don't know if making another PR to merge into this one is acceptable here.

@fuzzard
Copy link
Contributor

fuzzard commented Jan 22, 2023

Add it to this PR as just another commit.

@quietvoid
Copy link
Contributor Author

quietvoid commented Jan 22, 2023

Seems openssl-sys needs to be pointed to the native depend, somehow.
Using OPENSSL_DIR.

I can reproduce, just gotta fix.

Still results in

Failed to find OpenSSL development headers.

  You can try fixing this setting the `OPENSSL_DIR` environment variable
  pointing to your OpenSSL installation or installing OpenSSL headers package
  specific to your distribution:

      # On Ubuntu
      sudo apt-get install libssl-dev
      # On Arch Linux
      sudo pacman -S openssl
      # On Fedora
      sudo dnf install openssl-devel
      # On Alpine Linux
      apk add openssl-dev

  See rust-openssl documentation for more information:

      https://docs.rs/openssl

@quietvoid quietvoid force-pushed the libdovi branch 3 times, most recently from 151ff19 to f6dfeac Compare January 22, 2023 15:31
@quietvoid
Copy link
Contributor Author

With the latest commit series, I've been able to build all supported Android platforms.

@quietvoid
Copy link
Contributor Author

CBitstreamConverter indeed doesn't seem to be the right place for the Dolby Vision NAL manipulation, as it's mostly used for extradata.
In my case I don't care if CBitstreamConverter::Open fails, so I probably want to do a separate processor.

@sscobici
Copy link
Contributor

@quietvoid thank you for this PR. I was thinking about Kodi's ability to play DV .m2ts which has 2 hevc streams, but it would be another quite a big PR.
Do you think ffmpeg would also like this option while playing DV FEL files? Maybe this code change should be there as a filter/demuxer option?

@quietvoid
Copy link
Contributor Author

quietvoid commented Jan 25, 2023

I don't think FFmpeg would be interested in a bsf using an external lib.
They'd much rather have it internally.

After all, the parsing is already partially done.

I don't think Kodi currently uses the bsf API, and using it would also result in double allocations of every packet.
As they are usually run through CBitstreamConverter already.

So ideally the processing needs to happen both inside and outside CBitstreamConverter.

edit: Well it seems I am wrong and CDVDDemuxFFmpeg::ParsePacket actually uses a bsf to parse the extradata now.
I wonder if BitstreamConverter is still necessary these days.

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 13, 2023
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Mar 11, 2023
@quietvoid quietvoid force-pushed the libdovi branch 3 times, most recently from 6a2719c to 7592e99 Compare March 11, 2023 18:50
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Mar 14, 2023
@thexai
Copy link
Member

thexai commented Jul 16, 2023

@fuzzard
Is it necessary to upload depends sources (rustup, cargo-c) to mirrors before merge or can it be done later?

@fuzzard
Copy link
Contributor

fuzzard commented Jul 16, 2023

Yes put them on the mirrors.

With that said, if this is going to go in, is there really a need for an enable option? What's the purpose of not having it, build times only?
If yes, what is the actual increase in build times?

If it's negligible, remove optional and just force it

@thexai
Copy link
Member

thexai commented Jul 16, 2023

The build time increase only is when depends are build (not Kodi) and in Android this only happens when some depends changes, then I think is not much relevant.

@fuzzard
Copy link
Contributor

fuzzard commented Jul 16, 2023

It's absolutely relevant, as it then guides us on whether we need an enable option or not. Again, what's the point of the enable. Why would it ever be off? If the build time increase by a minute, remove it, if the build time increases by an hour, then it is an option to keep

Or is there some other reason why an enable is needed?

@quietvoid
Copy link
Contributor Author

quietvoid commented Jul 16, 2023

Again, what's the point of the enable. Why would it ever be off?

Originally it was intended to be optional at compile time, defaulting to off.
Then for CI it had to be enabled by default, but that was meant to be temporary.

The library isn't useful for most non-ARM Android targets, except those supported by CoreELEC & co. that have Dolby Vision support through vendor kernels.
But even then they maintain their own forks.

Otherwise it depends if we want the Rust toolchain to always be setup even though there isn't anything else using it in the codebase at the moment.

On my system, the toolchain and libdovi compile takes about 1min30 total at most (~24 threads AMD CPU).
On the CI the logs say it's around a minute or so.

I should probably fix the rustup install to only do the minimal setup for rustc, could save some seconds.

@fuzzard
Copy link
Contributor

fuzzard commented Jul 17, 2023

So it doesn't function on stock android?

@quietvoid
Copy link
Contributor Author

So it doesn't function on stock android?

What do you mean? It works if the device supports Dolby Vision through MediaCodec.
Otherwise it simply wouldn't be used but it should still build/link and Kodi would run fine.

@fuzzard
Copy link
Contributor

fuzzard commented Jul 17, 2023

The library isn't useful for most non-ARM Android targets, except those supported by CoreELEC & co. that have Dolby Vision support through vendor kernels.

Trying to understand this. Non-arm, as in x86/x86_64 android?

@quietvoid
Copy link
Contributor Author

Trying to understand this. Non-arm, as in x86/x86_64 android?

Yes, almost all the consumer Android devices supporting Dolby Vision are ARM.
I'm not sure if there are x86 ones at all.

I think I got confused with thinking CoreELEC did Dolby Vision on x86, but it actually seems to be ARM SoCs.

@fuzzard
Copy link
Contributor

fuzzard commented Jul 17, 2023

Ok, so thats a non issue for us then. We dont actively distribute x86/x86_64, its only done as a CI target, and outside of @joseluismarti i dont think anyone even knows if kodi runs on those arches currently.

What im trying to narrow this down to, is optional stuff is bad in general. It then doesnt run CI, it doesnt get distributed in kodi releaases (nightly/release apk's, play store), so then the code "rots". If the code works on regular android platforms (ie not custom kernels, not other custom shit, not only a single device in existence, etc), and someone wants to merge it, then build it as standard, and ship it as standard. If the feature doesnt add a crazy build time, ship it stock, remove the optional/enables. Enables are never tested outside of stock, so adding feature enables will just lead to maintenance/support hassles.

If it requires custom stuff (eg coreelec), then why merge into master, and they will just have to manage it downstream, otherwise the maintenance burden then gets shifted back to us. From what i understand, this isnt the case.

Just to say, im absolutely not against this, it doesnt affect me either way, but maintainability becomes an issue for the team at the best of times. Go all in, or not at all imo. if it works on a broad set of standard devices that users have that dont require tinkering, ship it.

@quietvoid
Copy link
Contributor Author

Given that x86 isn't really maintained, I agree that the optional flag should probably be removed.
I assume Kodi side it would probably be better in PLATFORM_REQUIRED_DEPS? Or should it stay optional there?

One more question before I make more changes:

Should the installed rustc version be made fixed?
Currently the latest stable is installed, so it may be different independently from Kodi's versioning.

@fuzzard
Copy link
Contributor

fuzzard commented Jul 17, 2023

I think you can leave it as platform_optional, and then you dont have to change any of the define code in core, as its not absolutely required for kodi to function, and it reduces the effort for you (ie just remove the configure enable and stuff around options in tools/depends).

As for fixed, i would lean towards yes. Again from a support side of things, rustc updates, and potentially breaks something because latest was downloaded by a user at some point in time just causes us grief. Just pick a version for now and if/when people want/need updated rustc for whatever, versions can be bumped later and breakages dealt when someone is actively maintaining that change.

@quietvoid
Copy link
Contributor Author

Removed the optional configure option. Now the Rust toolchain and cargo-c are always built for Android targets.

Updated the rustup install to use the minimal profile and to install fixed version (1.71.0 currently is the latest).
Also updated cargo-c but I don't know if the mirrors were already set.

@quietvoid quietvoid force-pushed the libdovi branch 2 times, most recently from 5500088 to 4f0dc55 Compare July 17, 2023 14:05
@quietvoid
Copy link
Contributor Author

Updated the depends files to use the Kodi mirrors.
It should work once libdovi-3.1.2.tar.gz is uploaded there.

@fuzzard fuzzard merged commit d3f1d4b into xbmc:master Sep 3, 2023
2 checks passed
@quietvoid quietvoid deleted the libdovi branch September 3, 2023 15:07
@abc123bac1
Copy link

abc123bac1 commented Dec 6, 2023

I had a quick question about the dolby vision compatibility mode introduced by this PR.

My understanding is that the EL is discarded to create a profile 8 dolby vision stream, but shouldn't this include the RPU information? I tried playing the BL_EL.mkv file from here and do not see any RPU response (flashing colours from 26 seconds onwards0

If the RPU response is ignored, is the stream being played the same as an HDR10 stream, but it just triggers DV mode in the TV?

Edit: Running kodi-20231128-4a41a5e4-master-armeabi-v7a.apk

@quietvoid
Copy link
Contributor Author

I had a quick question about the dolby vision compatibility mode introduced by this PR.

My understanding is that the EL is discarded to create a profile 8 dolby vision stream, but shouldn't this include the RPU information? I tried playing the BL_EL.mkv file from here and do not see any RPU response (flashing colours from 26 seconds onwards0

If the RPU response is ignored, is the stream being played the same as an HDR10 stream, but it just triggers DV mode in the TV?

No, those files have some extra metadata that is removed by the profile 8.1 conversion.
That metadata causes the flashes but it's not valid without the EL.

@abc123bac1
Copy link

So is my understanding correct that what is being played is just a HDR10 stream with the TV being flipped into DV mode?

@quietvoid
Copy link
Contributor Author

So is my understanding correct that what is being played is just a HDR10 stream with the TV being flipped into DV mode?

No, it's Dolby Vision profile 8.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants