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

H.263 decoder + initial video support #2173

Merged
merged 9 commits into from
Aug 21, 2021
Merged

Conversation

kmeisthax
Copy link
Member

@kmeisthax kmeisthax commented Dec 31, 2020

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

  • Write and implement a software H.263 decoder
  • Optimize it to play 480p video performantly
  • Add some sort of detection for out-of-order seeks (attempting to seek to a PFrame should instead seek back to the previous IFrame, else the video will be corrupted until the next IFrame)
  • Much ado about patents (see below)

Out of scope

Please note that the following features are not supported and are out of scope for this PR

  • FLV/F4V demuxing - only SWF embedded bitstreams are supported
  • On2 VP6, Screen Video, or H.264 decoding (the latter of which we'll likely have to proxy out to Media Source Extensions)
  • Non-Stage framerates (as SWF bitstreams always run at Stage framerate)
  • ActionScript representations of the video (neither AVM1 nor 2)

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.

@kmeisthax
Copy link
Member Author

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:

  • Expired
  • Involve error detection & correction modes that we don't support (US7006576)

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.

@kmeisthax
Copy link
Member Author

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 wasm-opt, which is responsible for optimizing our WASM binary beyond LLVM's ability to do so. On Ubuntu, and only Ubuntu, it chokes on the bounds checks of the idct_channel function of the H.263 decoder. I don't know why it's crashing or where it's crashing; reports on Discord are that the binary winds up getting stripped of anything useful. All I know is that if I remove the bounds checks, the program compiles correctly, even though that's drastically incorrect behavior and will crash on certain movies.

I've tried rewriting the bounds checks to change the loop ranges rather than using continue; but that doesn't appear to fix the problem.

As far as I'm aware, I either need to become a wasm-opt developer, rewrite the idct_channel function in a way that won't crash the optimizer, or file an issue and hope whoever currently maintains the optimizer can understand our own build system, rustc, and wasm-pack.

@kmeisthax
Copy link
Member Author

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.

@kmeisthax kmeisthax marked this pull request as ready for review January 20, 2021 04:24
@kmeisthax kmeisthax changed the title [DRAFT] H.263 decoder + initial video support H.263 decoder + initial video support Jan 20, 2021
}

if v > 255.0 {
return 255;
Copy link
Collaborator

@adrian17 adrian17 Jan 20, 2021

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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?

Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@torokati44 torokati44 Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With everything else being equal, measuring only the difference between the three clamp variants in the godbolt you linked, the same way as in my PR (on top of the changes of that PR):
image

@Herschel
Copy link
Member

Herschel commented Apr 21, 2021

I changed from an enum to a trait object VideoDecoder that wires into the actual codec; this made it easier to toss behind an "h263" feature, with an eye towards making all of the codecs optional.

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!

@kmeisthax
Copy link
Member Author

@Herschel A few months ago you mentioned wanting to migrate the ruffle_codec_h263 and ruffle_codec_yuv crates to a separate repository. Is this still the case? If not, I think we should merge this PR - otherwise, let me know and I'll start work on separating out the history of this PR.

kmeisthax and others added 8 commits August 21, 2021 13:52
@kmeisthax
Copy link
Member Author

I've heard nothing from @Herschel, but I've decided to follow his guidance and just split out the decoder into a separate repo. h263-rs is born: https://github.com/ruffle-rs/h263-rs

As a result, this PR is much, much smaller.

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

Successfully merging this pull request may close these issues.

4 participants