alfg / mp4-rust

MP4 reader + writer library in Rust! 🎥🦀
https://crates.io/crates/mp4
MIT License
295 stars 81 forks source link

Error: BoxNotFound(hdlr): Not found hdlr #102

Closed ajosecueto closed 1 year ago

ajosecueto commented 1 year ago

I'm getting this error using videos recoded from phone camera or screen recording -> BoxNotFound(hdlr) This error doesn't appear when i'm using videos downloaded from social networks or other platfroms.

Ocurs using mp4 = "0.13.0" but read_header works in mp4 = "0.12.0" with all kind of mp4 videos.

let f = std::fs::File::open(temp_file.path()).unwrap();
let size = f.metadata()?.len();
let reader = BufReader::new(f);

let mp4 = mp4::Mp4Reader::read_header(reader, size)?; // -> the error ocurs here 
mrjackwills commented 1 year ago

I am also running into this issue, working fine in 0.12, but 0.13 introduces some issue. I think I have narrowed it down to this code block, from meta.rs, but other than that I am at a loss.

When this is executed against my MP4 file, the hdlr_header.name is empty, the BoxHeader::read function is reading typ as 0, which it then can't convert into a BoxType.

 let hdlr_header = BoxHeader::read(reader)?;
        if hdlr_header.name != BoxType::HdlrBox {
            return Err(Error::BoxNotFound(BoxType::HdlrBox));
        }

I have also tried the Allow Hdlr to be not the first in the Meta box PR, and am still getting the same error.

EDIT: I have uploaded a "bad" 1 second mp4, https://www.mrjackwills.com/bad_mp4.mp4, it was shot with a Google Pixel 4a 5G phone.

DCNick3 commented 1 year ago

I can't download the mp4 file from this URL, it just serves me an HTML with a webapp redirecting to the main page

mrjackwills commented 1 year ago

I can't download the mp4 file from this URL, it just serves me an HTML with a webapp redirecting to the main page

Argh, sorry about that, I deleted the file by accident, and I need to fix something on the webapp to make it work correctly (PWA cache busting). As the GitHub file size limit is 10mb, I have a super short 3mb file here, also the same file in a zip, in case GitHub does anything funky with compression etc

https://github.com/alfg/mp4-rust/assets/32690432/e591052d-98f1-4b67-a4ef-7204234dc420

small_bad_mp4.zip

DCNick3 commented 1 year ago

Briefly looking at the file in MP4Box.js... It appears the file has an empty meta box in the moov box.

image

This code tries to read it as any other meta box, but as it's empty and doesn't have the hdrl box, it fails.

Now I guess is the time to look at the spec and try to figure out what is the correct behavior here...

DCNick3 commented 1 year ago

ISO/IEC 14496-12:2015 in section 8.11.1 clearly states, seemingly without any exceptions:

The 'meta' box is required to contain a ‘hdlr’ box indicating the structure or format of the ‘meta’ box contents.

It appears to be similar problem to #95 when implementers are interpreting the spec very liberally... In which case it's better to look at actual implementations like ffmpeg

mrjackwills commented 1 year ago

ISO/IEC 14496-12:2015 in section 8.11.1 clearly states, seemingly without any exceptions:

The 'meta' box is required to contain a ‘hdlr’ box indicating the structure or format of the ‘meta’ box contents.

It appears to be similar problem to #95 when implementers are interpreting the spec very liberally... In which case it's better to look at actual implementations like ffmpeg

Can I ask a slightly unrelated question. Where can one read the ISO spec, without having to pay over 200 Swiss Francs from the ISO official website?

DCNick3 commented 1 year ago

Can I ask a slightly unrelated question. Where can one read the ISO spec, without having to pay over 200 Swiss Francs from the ISO official website?

I don't think you are supposed to be able to. But you can find the PDFs in google if you look hard enough

mrjackwills commented 1 year ago

Can I ask a slightly unrelated question. Where can one read the ISO spec, without having to pay over 200 Swiss Francs from the ISO official website?

I don't think you are supposed to be able to. But you can find the PDFs in google if you look hard enough

Haha, I thought that might be the case

w-flo commented 1 year ago

My phone (Android-based) produces mp4 files like this, too.

It turns out they, like the mp4 file attached in a comment above, are not actually missing the hdlr box. Instead, something is odd with the meta box. Look at the file in a hex editor. As I understand the spec, the meta box should extend FullBox. That means the 4 bytes meta should be followed by 1 byte for the version number and 3 bytes for flags (all 0). However, in this case, the meta box seems to be extending Box only, which means there is no version number and no flags, and the next 4 bytes in the file are already the size field for the hdlr box (33).

So the call to read_box_header_ext() would probably need to be skipped in this case.

It's interesting that this seems to work in other software. I've looked at the ffmpeg code, it looks like for reading a meta box, they just scan through the file from the start of the meta box until they find the "hdlr" bytes, then jump back 8 bytes to reset the read position to the start of the hdlr box and parse it. If no "hdlr" bytes are found, the function returns without error.

Maybe some mp4 variants simply use Box instead of FullBox for the meta box? My phone says the ftyp is mp42, and the "small_bad_mp4.mp4" attached above says isom.

Edit: I suspect the quicktime format, which is of course similar to mp4, uses this meta format without the extended header. I do wonder why these files claim to be mp42 / isom, but still use the old quicktime format? Anyway, I guess the best way forward is to just handle this. Gstreamer seems to have some code that basically does:

w-flo commented 1 year ago

@ajosecueto @mrjackwills It would be interesting to know if #111 fixes this issue for you. I've tested the small_bad_mp4.mp4 from this issue thread, and that one appears to be fixed now.

ajosecueto commented 1 year ago

@w-flo it works, i tested with some videos and works. No more Not found hdlr.

mp4 = { git = "https://github.com/alfg/mp4-rust.git", rev = "55ee392b73801d5aff41dbde36455917e0ac2d19" }
mrjackwills commented 1 year ago

@ajosecueto @mrjackwills It would be interesting to know if #111 fixes this issue for you. I've tested the small_bad_mp4.mp4 from this issue thread, and that one appears to be fixed now.

Sorry, I've not had proper access to my computer or the internet for a while, but I can confirm that it appears to now work when using the following dependency;

mp4 = { git = "https://github.com/alfg/mp4-rust.git", rev = "55ee392b73801d5aff41dbde36455917e0ac2d19" }
w-flo commented 1 year ago

Great! Thanks to both of you for testing. Hopefully this will give the PR a good chance of being merged.

alfg commented 1 year ago

Thanks, all. I've been super busy, but I'll try to make time to review and merge soon. Sorry for the wait!

mrjackwills commented 1 year ago

@alfg @w-flo I only tested it against "bad" mp4's, haven't looked into the source of the pr fully, but assume "good" mp4's will still work as before, can test again later

alfg commented 1 year ago

@mrjackwills Thanks. I have a set of "good" mp4s that I use to validate that I will run through. I've been meaning to create a new set of integration tests for these as well.

alfg commented 1 year ago

95 and #111 are merged. Feel free to try again using the latest in master and I will create a release.

mp4 = { git = "https://github.com/alfg/mp4-rust.git", rev = "d6c38642de48360693132fd07552a454016d71c3" }
or
mp4 = { git = "https://github.com/alfg/mp4-rust", branch = "master" }
w-flo commented 1 year ago

This issue is fixed for me when using the master branch, thanks a lot @alfg!