Add mp4 support #1419
Add mp4 support #1419
Conversation
|
After running the server, I just realized why you don't like "illicit NFT" formats. I think you basically have to reindex, lol |
|
@cryptoquick I want to do a check that the client only allows H.264 in MP4 when it uploads. If you want to look into doing that feel free, I think there's an MP4 parsing crate somewhere, otherwise feel free to leave this open and I can add that later. You shouldn't have to reindex, illicit content types are indexed just fine, it's just the frontent that doesn't show them. However, we did change the database schema on master, so that might be what you're running into. |
|
Can you tell why this didn't load? |
|
Yup, We could add it to the Then later when we check the codec in |
|
See the route in server.rs for |
Co-authored-by: Casey Rodarmor <casey@rodarmor.com>
|
What I saw in preview was really weird to me. Maybe good for testing? I couldn't figure out what it was for or how to use it. Also, just so I don't assume, why the h264 requirement? Compatibility? |
Yah, that's correct. Cross-browser compatibility of containers and codecs is really bad. The reasonable choices for good compatibility are H.264 in MP4 and VP8/VP9/AV1 in WebM. MP4 is containers used with many codecs, and many of those codecs are unsupported. In particular, MP4 is often used with HEVC, which both doesn't have great support (doesn't work in firefox for example) and is patent encumbered. WebM though, even though it's a container, not a codec, is almost always used with broadly supported codecs. A user might preview a HEVC in MP4 inscription with their browser, but then find that many other browsers won't display it after publishing it. |
|
It's looking like adding the codec check is actually gonna be pretty easy, I should have a commit for that soon. |
src/inscription.rs
Outdated
| let extension = path | ||
| .extension() | ||
| .ok_or_else(|| anyhow!("file must have extension"))? | ||
| .to_str() | ||
| .ok_or_else(|| anyhow!("unrecognized extension"))?; | ||
|
|
||
| if extension == "mp4" { | ||
| Media::check_mp4_codec(path)?; | ||
| } | ||
|
|
||
| let content_type = Media::content_type_for_extension(extension)?; |
There was a problem hiding this comment.
Let's move these three checks into Media::content_type_for_path, e.g. extract the extension, check the codecs, and then return the Media value.
There was a problem hiding this comment.
Good thinking! That'll be cleaner. I'll rename the method, and do as you suggested.
src/media.rs
Outdated
| #[test] | ||
| fn check_mp4_codec() { | ||
| assert!( | ||
| Media::check_mp4_codec(Path::new("bitcoin-pup.mp4")).is_ok(), |
There was a problem hiding this comment.
We could use a negative test too, to check that an MP4 with an unsupported video codec fails the check.
There was a problem hiding this comment.
I'll look into it.
src/media.rs
Outdated
| if let TrackType::Video = track.track_type()? { | ||
| if let MediaType::H264 = track.media_type()? { | ||
| return Ok(()); | ||
| } else { | ||
| return Err(anyhow!("h264 format supported, only")); | ||
| } |
There was a problem hiding this comment.
I have no idea what can go into an MP4, but let's check all tracks, and also put the unsupported media type in the error message:
if let TrackType::Video = track.track_type()? {
let media_type = track.media_type()?;
if let MediaType::H264 != media_type {
return Err(anyhow!("Unsupported video codec, only H.264 is supported in MP4s: {media_type}"));
}
There was a problem hiding this comment.
If you look at the enums for either TrackType and MediaType, I believe this code will behave as expected... So long as any one of the video tracks doesn't contain something that isn't encoded as h264, it returns Ok, otherwise, it returns an error.
That said, it's a good suggestion to return the media type.
There was a problem hiding this comment.
I just realized, the code I had before had slightly different logic than when I explained what I wanted... I'm gonna redo some of that logic also.
… mp4 codec detection logic. Add negative mp4 test case.
|
@casey icymi, this pr is rfr |
|
@cryptoquick Nice, thanks for the ping! I tweaked a little, just the tests. I moved the example files into Thanks for this, vv nice addition ❤️ |
ILLICIT NFT ALERT
Closes #1350