-
-
Notifications
You must be signed in to change notification settings - Fork 809
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
Add H.264 decoding by utilizing OpenH264 from Cisco #14654
Add H.264 decoding by utilizing OpenH264 from Cisco #14654
Conversation
This is what I use to test this at the moment: I hope it's okay to upload here. Despite the name, it now loads an FLV file. It's originally from here: https://permadi.com/2010/02/flash-playing-flvf4v-videos-with-script/
|
50cbcdb
to
7b9cfe9
Compare
7a9b39d
to
9f5213a
Compare
9f5213a
to
d0a74d3
Compare
94a9b3d
to
ae7005d
Compare
bab9a41
to
ff4890b
Compare
9c9c068
to
8d13939
Compare
18ec491
to
1aad82a
Compare
The VideoDecoder part of WebCodecs (and many other APIs) are only available in secure contexts, right (they're also not yet available in Firefox but I'm sure that will change)? Will this be the case in insecure contexts too? I suppose browsers are moving away from allowing insecure sites at all. |
Certainly, since it already works if enabled in
I have no idea. Anyway, we can keep the current solution if VideoDecoder is not available for whatever reason. |
f17d412
to
a49f46d
Compare
a49f46d
to
2416cac
Compare
526525a
to
5cd122d
Compare
@@ -0,0 +1,11 @@ | |||
// bindgen ../openh264/codec/api/wels/codec_api.h --no-prepend-enum-name \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done through a build.rs
script, instead of committing the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a technical PoV, of course.
Then, we'd have to vendor (or perhaps obtain in some other way) the C header in question at build time.
I think the license within it allows vendoring, and the header in itself should not be problematic regarding patents or royalties.
That is a smaller file to check in, at the price of another dependency and a smidge longer build time.
Would you prefer that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please weigh in on this one too, @Dinnerbone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't care enough to block this PR. If someone feels strongly about it, let's follow up in another PR later.
desktop/src/player.rs
Outdated
if cfg!(feature = "external_video") && preferences.openh264_enabled() { | ||
use ruffle_video_external::backend::ExternalVideoBackend; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the effort to feature gate this, but this is a compile error if the feature is turned off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thank you! Added an inner block that is conditionally compiled.
video/external/src/backend.rs
Outdated
|
||
let (filename, md5sum) = Self::get_openh264_data()?; | ||
|
||
let filepath = std::env::current_dir()?.join(filename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not use current_dir please... that could be anywhere
Maybe our config storage place for now?
Or absolute worst case, the folder that the ruffle executable is in... but that's not likely to be the same as current_dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe our config storage place for now?
Ummh, I'd find it weird to put anything "executable" in there.
Or absolute worst case, the folder that the ruffle executable is in...
That should be fine IMHO.
but that's not likely to be the same as current_dir
Yeah, good point, I'm a bit too Linux-CLI-minded... 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to:
let filepath = std::env::current_exe()?
.parent()
.ok_or("Could not determine Ruffle location.")?
.join(filename);
This way, we don't need to touch .gitignore
either, as the lib will be put into target
.
5cd122d
to
91e7c46
Compare
Some further minor TODOs:
|
91e7c46
to
874b18c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works on mac and windows.
Slow on my mac but profiled on windows and 2/3rds of the time is spent in openh264 - the other third is to_rgba
which we can look at offloading more of to gpu later maybe
Featuring the legendary Big Buck Bunny.
874b18c
to
e5045a3
Compare
https://hoffmeister.li/tv/stream1.html </script> <script src="[https://hoffmeister.li/flash-games/ruffle/ruffle.js](view-source:https://hoffmeister.li/flash-games/ruffle/ruffle.js)"></script>does NOT work :( |
@Benman2785 This PR is DESKTOP-ONLY, it does NOT affect the web build of ruffle. |
@Benman2785 The web implementation of H264 decoding will be based on WebCodecs API and is still work in progress. |
@Benman2785 The reason is that if you don't use the OpenH264 prebuilt binaries provided by Cisco, you will have to pay for the patent fee to MPEG LA by yourself, and Cisco hasn't provided the WebAssembly-targeted prebuilt binaries of OpenH264, so we have to use the built-in decoders in the browser(which we haven't done yet). |
This is very much a WIP.Progresses #723.Basically pushing only for visibility.Will partially replace #12539 (first only on desktop, in the future, using WebCodecs, entirely).AFAIK this is the only legal way (without us or our users having to pay royalties to
MPEG LAVIA LA) to do this with reasonable portability on desktop, but IANAL. See this little explainer video: https://vimeo.com/79578794The code is somewhat based on the
openh264-rs
crate, see ralfbiedert/openh264-rs#43.YUV->RGB colorspace conversion is done by our trusty handcrafted code, like for H.263 and VP6[A]. This means that it will always assume BT.601 coefficients, even though it could theoretically, possibly be BT.709 or something like that. But the difference is probably subtle enough anyway.
TODOs, in no particular order:
configure_decoder
(or similar) method to the video backend/decoder traits.Currently the preload method is misused to pass the "extradata" to the decoder.Index
es for proxied and handled videos in the ExternalBackend.OpenH264-license.txt
upon creation.--enable-openh264 [true|false]
SBufferInfo
struct from the decoder.TODOs for later:
<video>
element would be a feasible fallback - probably not, but who knows.I plan to put this into the same
ExternalVideoBackend
added here. Also with the "other ideas" below. Spares us from duplicating the proxying logic. The Android backend will have to be different though, as it's somewhere else.Somewhat less closely related TODOs for later:
Other ideas: