-
-
Notifications
You must be signed in to change notification settings - Fork 843
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 VP6[A] video decoding support #5369
Conversation
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.
This more or less looks good to me. I'll merge after the repo transfer (so you have a chance to update the Cargo ref).
I merged the h263-rs PR you mentioned, so you might want to bump that at some point. You should also ensure the documentation on licensing makes it to LICENSE.md
's third-party software table (which, itself, probably needs updating).
4784faa
to
78da7f1
Compare
I have transferred the repo, updated the Cargo.toml ref, and added a line to LICENSE.md about this. Thank you for merging the h263-rs PR. I will submit a separate dependency bump for that soon - I didn't want to bunch that up into these changes. |
I have just now realized that we might be waiting for each other, because I was entirely unclear about the h263-rs git bump. |
01ef816
to
c711676
Compare
c711676
to
69f6caf
Compare
Getting a panic on the following file
|
Gated behind the "vp6" feature, enabled by default. Utilizing a heavily stripped-down version of the NihAV project, retaining only the VP6 decoder, relicensed under MIT. Including VP6WithAlpha decoding, proper FrameDependency reporting, and cropping the unwanted encoded pixels on the right/bottom manually.
69f6caf
to
f9d2522
Compare
Thanks a lot for checking, I found the cause of the panic and fixed it. I didn't skip the first 24 bits encoding the offset to the alpha frame, which precede the proper VP6 header. The decoder itself does this on its own, which is why it worked with fixed sizes. |
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.
Thank you!
@torokati44 Fantastic work, thank you so much! Being able to view embedded videos in flash loops does a lot! |
This is an alternative to #3004.
In contrast to that:
clang
, no iffy FFI, no funky business with adding a quasi-libc on web, and no CI troubles.This is more in line with what the H.263 decoder added, which was about 80k, although that was including the colorspace conversion code.
h263-rs-yuv
for the colorspace conversion.Most likely as a result of this, it is measurably slower, at least on web. See for example: https://z0r.de/2289
Merging yuv: Remove chroma interpolation, use iterators h263-rs#13 (then bumping the crate git ref) and web: Enable the bulk-memory WebAssembly extension #5225 should alleviate this somewhat.
Some more size reduction can be achieved by dropping a few more parts that are not strictly necessary, but I think this is already acceptable.
Thanks again to the author: https://codecs.multimedia.cx/nihav-relicensed-code/
unsafe
blocks, aside from a few really small ones, in a single file, in one of the the decoder crates.Similar to that:
vp6
feature toruffle-core
, enabled by default.VP6WithAlpha
decoding andFrameDependency
(keyframe) reporting.bounds
.I plan to move ownership of https://github.com/torokati44/nihav-vp6 to the Ruffle organization after review, but before merging.