-
-
Notifications
You must be signed in to change notification settings - Fork 830
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
H.263 decoder + initial video support #2173
Conversation
a525e69
to
5c5d128
Compare
So, I've looked through both the Wikimedia H.263 list as well as the ITU H.263 disclosures. As far as I can tell, all publicly-disclosed H.263 standards-essential patents filed in the US are either:
For good measure, I also searched for any patents mentioning Sorenson or H.263 and uncovered one patent owned by MediaTek (US8180166B2) which covers the use of IDCT level truncation to decode Sorenson bitstreams with non-Sorenson hardware. This PR doesn't do that; however, it would potentially prevent us from distributing code that attempted to leverage hardware H.263 decoders by providing them converted bitstreams. I was not able to find evidence of extant H.263 patent pools. However, MPEG-4 Visual / ASP has a related history to H.263; they use similar coding tools and thus would have similar patent exposure. So I went onto MPEG-LA's site and read through the MPEG-4 patent pool's list of licensed patents. The vast majority of them were expired; the two US patents that were not expired cover features relating to bidirectional prediction that I have not implemented in this PR. (H.263 has several extensions providing functionality covered by said patents; but none of them are required to decode Sorenson bitstreams and thus generate errors if you try to decode them.) I also tried to find any other Free Software projects' discussions of the patent status of H.263. ffmpeg's official policy regarding patents is roughly "we don't read them, there's too many patents to actually check if our software is infringing any of them, and ITU and MPEG standards just say that they're there". Not helpful in the slightest. I also tried to see if H.263 was in the GStreamer "ugly" package, but it turns out they just use ffmpeg and thus distribute it in a separate package of ffmpeg wrappers. Mozilla's web video codec guide claims H.263 is patent encumbered; but it just links to the ITU patent disclosure lists that I already checked. I personally think that H.263 - at least, the unextended, weird version Sorenson uses - is old enough that all of the relevant patents should have expired. I've found no evidence to the contrary; I can find no live patents covering standards-essential technologies and I know of nobody trying to collect money for their use. |
So, the Ubuntu test failures on web are not transient and appear to imperil this PR. The test failures are ultimately caused by a software package called I've tried rewriting the bounds checks to change the loop ranges rather than using As far as I'm aware, I either need to become a wasm-opt developer, rewrite the |
I've been doing some back and forth with @torokati44 and he's massively improved the IDCT performance, to the point where 480p video plays back just fine. I'm going to release the draft lock, even though I don't expect this to get merged right away. |
codecs/yuv/src/bt601.rs
Outdated
} | ||
|
||
if v > 255.0 { | ||
return 255; |
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.
After saturating conversions became default in Rust, clamp
can be simply (v + 0.5) as u8
, right?
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.
Hah, good point! IMHO, yes.
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.
While nice and concise, I find ithis to be a lot slower. No idea why.
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.
Oh, sorry, I compared it against an already modified local version: (v + 0.5).max(0.0).min(255.0) as u8
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 these explicit .min().max()
calls are not as careful/complete as the one that comes with the cast (when used on its own), but they are enough for the compiler to drop these checks from the cast?
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 think I got lost, not sure which you mean is faster in the end :)
But lyon author had a similar observation that .max()
is sometimes slower than expected:
https://github.com/nical/lyon/blob/4c31d8c9907f03ad5962d02e22e2783bfdfbfdb0/tessellation/src/fill.rs#L28
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.
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.
See: kmeisthax#12
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.
c035321
to
ded159b
Compare
I changed from an enum to a trait object Still not totally decided if I want to pull this out into a separate repo or not, but I'll get this merged tomorrow. Thanks for the patience! |
@Herschel A few months ago you mentioned wanting to migrate the |
…cialize per-renderer
…ma buffers to the next pixel in that direction.
Change VideoStream to use a VideoDecoder trait instead of an enum. This will make it a little easier as more codecs are added, and allow us to easily enable/disable codecs behind features.
I've heard nothing from @Herschel, but I've decided to follow his guidance and just split out the decoder into a separate repo. As a result, this PR is much, much smaller. |
This PR comprises a pure-Rust software H.263 decoder, a new backend type for interfacing with all future video decoders, and a very basic implementation of the
Video
display object type, only suitable for playing embedded video streams with no interactivity.I'm marking this as a draft PR because people are already testing this branch, so I might as well announce it properly and get it into CI. The decoder is horribly badly optimized - even in release mode 480p video takes 8-16x longer to decode than ffmpeg. I ultimately want to write GPU shaders that do most of the per-pixel maths, but if I can get the CPU side to decode 480p video then I think this will be good.
I've explicitly designed the Video Backend to be able to support external video decoding APIs, mostly so that Desktop can proxy out to system codecs in the future. It may also be useful going forward for H.264 support on web.
Progress
Out of scope
Please note that the following features are not supported and are out of scope for this PR
Patents
Before I release the draft lock, I need to do at least a cursory review of any remaining known H.263 patents to determine if they cover material this PR can handle. From [https://meta.wikimedia.org/wiki/Have_the_patents_for_h.263_expired_yet%3F](this list on Wikimedia Meta) there appear to only be a handful of active patent grants, and most of them would cover later versions of H.263 that Sorenson never bothered to support. For example, the Nokia patent that is supposedly live until 2023 covers certain error recovery techniques and the Supplemental Enhancement Information extension that Sorenson doesn't support; it would likely not cover this PR.
If there is still patent coverage, then we'd either need to delay merging of the PR, or add new config options to allow building Ruffle without certain codecs (and Public API changes to negotiate codec support). I don't know if we'd need to do this for VP6; the entire point of Macromedia using On2 codecs instead of ISO/IEC standards was that they were deliberately designed to be royalty-free. H.264 will likely be covered for years to come and my plan for that codec is to just forward the bitstream straight to MSE, as it still has broad (and accelerated) browser support.