-
-
Notifications
You must be signed in to change notification settings - Fork 832
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 video decoding by utilizing an external crate that bundles parts of FFmpeg #3004
Conversation
e2fc8d9
to
af20eac
Compare
Apart from getting the CI to pass on Windows, I might start to split this into a separate repo already, and instead of bundling libav code, refer to it as a git submodule. |
Great work! Having video support in Ruffle would do a lot! |
Well, thank you, I sure am also looking forward to it myself, but there are certain standards that are better met by the code that gets accepted, and this isn't quite there yet... :) |
2c5984f
to
44c44cf
Compare
9a1c7a9
to
ad23ba0
Compare
Hi, FFmpeg developer here. The nellymouser decoder was also based on our code, and although credit was given, it was relicensed without permission. Since it's not a lot of code and parts were taken from nelly2pcm I chose to not make an issue out of it, but this is a huge decoder. EDIT: I noticed the new version of the PR links to Libav instead of oxidizing the C code. Linking to Libav is still going to result in a license violation, since it's LGPL licensed, and even then, why Libav and not something that's actually maintained? |
Hi @cyanreg, Certainly our intention is to obey all license requirements as required and give proper credit (simply none of this would be possible without the great work from FFmpeg and other projects). This specific PR has not been reviewed yet and is still a WIP; in the meantime I will take the public repo private until we can obey the proper licensing requirements. (@torokati44, can you look into resolving the issues mentioned above?) As for Nellymoser, the original code is MIT licensed, and our Rust port is also MIT licensed. It includes the original copyright notice from the original authors. Our intention is to not relicense it, but keep it under the MIT license. I see we do not explicitly mention that this component is only MIT licensed in this Ruffle repo, where we link to the library; if we explicitly note that this component is only MIT licensed in this repo, and additionally copy over the MIT copyright notice into our LICENSE, is that satisfactory? Thanks for the help and patience, I really do appreciate it; I'm trying my best to obey the licenses properly and make sure everything is correct. |
Hi!
I'm not sure what exactly are you referring to here. All libav code is (since about two days ago) referenced verbatim, as a git submodule - it's not even directly included in the repo itself. I thought doing this was somewhat of a common practice. Of course I'm happy to give all credit and attribution for the decoder to the actual authors. In what form do you think this would be done best? Also, no rewriting of any C code into Rust was done here, only binding declarations and other glue added.
Huh, I was still under the impression that the libav fork went on being the more active. I guess we should switch over to that project then! Unless you have any objections, of course.
The currently referenced commit is from April 2019. And indeed, it seemed somewhat old to me, but not that old, and I just thought it was because it is "done" - I was wrong...
Yes, this I even mentioned in the original description! It just wasn't wired up yet.
That was entirely @relrelb's work, I know ~nothing about it. |
There wasn't any "oxidation" done by me to any libav code at any point in time. I simply started out with copying unmodified C source files from libav into my fork, compiling them with clang and linking them into Ruffle. The copied files are not here anymore, because:
So, is there even any way to include any libav (or ffmpeg!) decoders into Ruffle, with its current license?
See my previous comment, I had the wrong impression about the relative activity of these projects. Seeing your points, now I think that - if anything - ffmpeg should be used instead. |
If my understanding is correct, we should be able to link to an LGPL library in our MIT+Apache2 licensed project due to the linking exception provided by LGPL. The VP6 repo (oxidized or not) must be licensed under LGPL. When we link to it, we can specify that this dependency is LGPLv3 and include the LGPL+GPL license text in this repo's LICENSE.md.
Yes, we should use FFmpeg's decoder instead. |
What an utter mess. This PR contains the H263 Rust decoder (WHY?!). Since VP6 has some common parts with H263 I thought this PR implemented a Rust version of a VP6 decoder. I was not looking at the commits, only at the full diff. It didn't help the H263 code contains few references to H263 in its own code, so it's not easy to tell from just a random chunk of IDCT and coefficient parsing code. In which case, since no code is taken, I'm fine with it. Just has to comply with the LGPL the wrapper is linking to.
This hasn't actually been the case at any point AFAIK.
We have dozens of daily commits and thousands of monthly emails on the mailing list. |
It is marked as DRAFT for a reason. :)
This is explained in the original description. The only reason for this is that the base video infrastructure itself (that this PR depends on) was developed closely together with the H.263 decoder in #2173, and neither is merged yet. Now that the non-decoder parts were separated into #3117, only that would need to be included in this PR. And after it is accepted, not even that. By the way, AFAIK the H.263 decoder was written entirely from scratch, not using any ffmpeg/libav code, only the spec itself. But @kmeisthax can correct me if this is not the case.
Understandable, this really isn't that clean at the moment.
Great!
At the moment the wrapper compiles the referenced libav/ffmpeg C sources (not all of them, just what's needed for VP6) into itself during build. So, essentially, links to them statically. This makes it impossible to swap the wrapped decoder out after it is built - I am, however, completely fine with the wrapper also being LGPL.
While I agree, and have also considered this myself, I wouldn't really expect ffmpeg to be installed and available on most systems (except for Linux), and since this would slightly complicate the build process, I don't consider this a priority. In the future, I might explore this option as well.
And we are grateful for your efforts, and certainly wouldn't want to diminish them!
I'm sorry about that. We'll try our best so Ruffle won't ever make it onto this list. :) Another, slightly unrelated note: This wrapper will also include qsort.c from musl-libc, but as that is already MIT licensed, hopefully this should be only a matter of an additional copyright notice. |
If it's possible to swap the wrapper itself (which would contain the compiled decoder code), then it's fine. That's why I mentioned the wrapper ought to be LGPL.
Make it a configure-time option or just autodetect it? You're already using the proper external API, so it's just a matter of not building the internal code and instead linking to the system library. |
I can confirm that the |
FYI: I rebased this PR on top of #3117, this time without squashing its commits. |
9afb239
to
0acd3b6
Compare
So. I have updated the FFmpeg submodule of the decoder crate to current Once the web tests on macOS are re-enabled in the main Ruffle repo, some tricks will have to be taken over from there (installing After rebasing, and adhering to the new way of doing this (such as @adrian17 had concerns about the I'd gladly hand over this repo (https://github.com/torokati44/vp6-dec-rs) to the organization, so https://github.com/ruffle-rs/vp6-decoder-libav can be deleted. Instructions for installing some additional dev dependencies should be added to the Wiki after merging this. I'm now removing the DRAFT marker from this PR. |
56eb4f6
to
f67b16f
Compare
Update: The |
553bfdf
to
9e7b954
Compare
Do you happen to know, @cyanreg, that in case of VP6, which variant of 4:2:0 chroma subsampling is used? I'm asking specifically about the location of the chroma samples. I suppose it's one of the bottom ones on the figure in this section: FFmpeg sets/leaves |
VPX until VP9 came from the wild west era before web colorspace standardization where any coefficients that sort of made yuv were yuv. Including labelling YCgCo as YCbCr. They really didn't care much about chroma location, and who knows what primaries/colorspaces/transfer functions they actually used. VP8 was the first to specify the colorspace that ought to be used, because it started to matter, and seeing as how it's closely following VP6, my guess is it's the same. VP8 used BT.601 primaries/colorspace/transfer function, which is the same as JPEG, but whilst JPEG defined centered chroma samples (FFmpeg has a neat illustration and description), centered chroma requires interpolation, which is expensive on CPUs, so I'm very sure they'd have used top-left chroma, seeing as that's what the popular codecs of the day used, and therefore video editors and devices, so all sources. As it was the wild west, I seriously doubt they'd process the chroma to JPEG-style centered samples. |
Okay, I see, thanks! I also found that ASCII art comment in FFmpeg, and I too think it's really nice and useful. :) And yeah, the SWF spec mentions that VP6 uses the same reduced range BT.601 colorspace as H.263, so we can use that. But then again, it also says that H.263 is 4:2:2 chroma subsampled, which it isn't... Oh well, it's not the most flawless spec, nor is this the most straightforward notation, if you ask me. At least the colors look entirely the same to me when doing the conversion with the code made for H.263, as they do in Flash Player, or any media player using FFmpeg (including this branch here). So, basically, my understanding is:
Is this correct? |
You should always do bilinear interpolation to upscale chroma to luma's resolution. Option 2 is a no-go, because nearest-neighbour upscaling on anything looks really really bad to anyone who notices. MPEG-4 (H.263) uses left-aligned chroma (so |
Err, right, I got it wrong.
No, that's by far the worst - if you're blowing up a pre-converted image with misaligned chroma, it'll look worse than blowing up all planes to display resolution before conversion, and converting them to RGB. In the first case, your chroma offset error of 0.71 pixels (
Maybe you should look into changing the software converter? It shouldn't be that much work to add a configurable chroma offset and change the sampling weights (if it's using something like a kernel, though it probably isn't for bilinear). |
Yep, this is exactly what I'm aiming for. Although Flash has a property that can switch the RGBA scaling between nearest and bilinear (AFAIK), but I'm fairly certain that that's done after the colorspace conversion, so not really in scope here.
Originally, the converter for H.263 was based on a naive and generic Now I'm trying to reuse/adapt that code to do the conversion for frames decoded from VP6 (with the appropriate interpolation), and this brings us back to my original question again: what is the correct way here? Do I have to resort to tediously observing the rendering of manually crafted test videos, and comparing the different methods, to see which one matches Flash Player? :/ |
https://code.videolan.org/mahanstreamer/nihav/-/blob/master/nihav-duck/src/codecs/vp6.rs might be helpful. Even written in rust to top off the cake |
Yeah, this looks wonderful! Thanks! |
And about the chroma sample positions: It looks to me now that Flash Player just uses the same chroma sample for the whole 2x2 pixel group, without any interpolation, for both H.263 and VP6 videos, so maybe I shouldn't worry about this at all, and do the same thing. It will be a lot faster, and similar to Flash Player, even if not as pretty... |
Yes, my friend has reached out to the author, waiting for a response. |
Apparently he expressed implied approval, but i would ask him at https://codecs.multimedia.cx/2021/09/vp6-simple-interframe-encoder-done/#comments to double make sure. |
…ndency With the "allow-lgpl" feature enabled on it.
With a new VideoDecoder trait implementation, including VP6WithAlpha decoding, proper FrameDependency reporting, and cropping the unwanted encoded pixels on the right/bottom manually.
By adding MSYS to $PATH (for clang), and setting up the MSVC development environment in the shell (for link.exe).
9e7b954
to
d52af2b
Compare
An alternative, #5369 was merged instead of this. |
This is very much a
WIP POC RFC OMGPR.(With #3117 merged, there aren't any "extra" commits in here anymore.)
Because this is (I feel like) a fairly heavy change set (both in technical and legal aspects), I'd like to get some feedback about it, before fully polishing it, and hopefully together an acceptable state for merging can be achieved.
While rough around the edges, it's already complete enough that it can be seen working on z0r.de: 7055, 7340, 7617, 7872, and many others.
(I'm hoping that deploying "unofficial forks" like this in the public is not problematic?)
Most functionality is in an external crate. The rest of the description applies to the contents of that separate crate.
At the moment, that crate lives in this repository: https://github.com/torokati44/vp6-dec-rs
In the future, this will be migrated under the Ruffle organization.
The C code is built by a fairly straightforward
build.rs
using thecc
crate, which automatically handles native compilation or targeting WASM for the extension/selfhosted variant, using a static config.For the WASM target, instead of relying on Emscripten or something similar, a (handmade) tiny fraction of a libc is included as well, just enough to make the decoder compile and work.
This same libc stub is also utilized on Windows - it was easier this way than supporting compilation with MSVC as well.- EDIT: This is no longer true.Binding declarations are based on those generated by
bindgen
, but in the end, only a tiny portion of those were kept - and some other parts didn't work well then targeting WASM.This adds clang as an additional dev dependency, which hopefully isn't too big an issue. Version 7 is for sure too old, 11 is confirmed recent enough - but at least not a whole Emscripten SDK is required.
I opted not to use any of the already available ffmpeg binding crates, because they all seemed to have issues with cross-compilation, and doing this without them wasn't that bad after all.
Some things that should be fairly straightforward to add, but aren't done yet:
Trimming the "padding pixels" off from the right and bottom edges, they are needed only for compression.HorizontalAdjustment
andVerticalAdjustment
fields are not present.Also, I've checked, and even if the size in the header is not an integer multiple of 8 or 16 (the block sizes), the results already match that of Flash Player nicely. Most likely, the actual frame size is also part of the frame data fed to the decoder in FFmpeg, which then handles it properly.unsafe
blocks can crash or leak or do anything nasty.