Eyevinn / mp4ff

Library and tools for parsing and writing MP4 files including video, audio and subtitles. The focus is on fragmented files. Includes mp4ff-info, mp4ff-encrypt, mp4ff-decrypt and other tools.
MIT License
462 stars 84 forks source link

Multiple mdat boxes lead to panic #371

Closed alttagil closed 2 months ago

alttagil commented 2 months ago

I came across mp4 files with two mdat boxes: first is the real one and the next is empty. All players and wowza were playing it all right but mp4ff reads such file and rewrite file.Mdat with second empty one. I propose these changes to file.go for fixing it:

    case *MdatBox:
        if !f.isFragmented {
            if f.Mdat == nil || f.Mdat.Data == nil {
                f.Mdat = box
            }
        } else {
tobbee commented 2 months ago

@alttagil Thanks for the report. This is a weird case, but apparently allowed by the spec. There may also be multiple mdat boxes containing data, or references to data in another file. The goal of mp4ff is not to be the most general parser/processor possible, but this is an easy enough change that I can add something similar to the code change you suggest. I think, I'll rather use the size of mdat than a nil pointer in the test, though. I suppose your empty mdat box has the size properly set to 8?

alttagil commented 2 months ago
[ftyp] size=8+24
  major_brand = isom
  minor_version = 200
  compatible_brand = isom
  compatible_brand = iso2
  compatible_brand = avc1
  compatible_brand = mp41
[free] size=8+0
[mdat] size=8+20990219
[mdat] size=8+0
[moov] size=8+72942
  [mvhd] size=12+96
    timescale = 1000
    duration = 127318
    duration(ms) = 127318

here's the link to the file: https://www.dropbox.com/scl/fi/su2erq0990vih6tqdgti6/rabbit.mp4?rlkey=arn4nhci75we4tbx1zij3c2eh&st=vra0xays&dl=1

tobbee commented 2 months ago

I wrote a bit more complex fix where Mdat will point to a single empty mdat box, but it will be point to a non-empty one if such a box is added. A second non-empty box will return an error. This should hopefully avoid panics.

alttagil commented 2 months ago

it seems that your PR doesn't prevent rewriting non-empty box with empty one

tobbee commented 2 months ago

@alttagil Yeah, you're right. I didn't write a test to check the behaviour. Thanks for checking!

The code has now been fixed and there is a a new test that checks the various cases with mixed empty and non-empty mdat boxes.