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

fix: Build ESDS box missing data property #378

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hughfenghen
Copy link

@hughfenghen hughfenghen commented Jan 26, 2024

closes #341

@DenizUgur
Copy link
Collaborator

Any particular reason you need the raw data and not the parsed descriptor?

@hughfenghen
Copy link
Author

Any particular reason you need the raw data and not the parsed descriptor?

Missing the 'data' field will result in an incorrect box size, causing file parsing errors.

Some video players will lose audio when playing MP4 files that lack an esds box.

@DenizUgur
Copy link
Collaborator

The link you've sent is for writing the boxes. Parsing of the boxes "should" skip the box if parsing of that box was not completed successfully.

mp4box.js/src/box-parse.js

Lines 110 to 118 in 685f491

box.parse(stream);
diff = stream.getPosition() - (box.start+box.size);
if (diff < 0) {
Log.warn("BoxParser", "Parsing of box '"+box_type+"' did not read the entire indicated box data size (missing "+(-diff)+" bytes), seeking forward");
stream.seek(box.start+box.size);
} else if (diff > 0) {
Log.error("BoxParser", "Parsing of box '"+box_type+"' read "+diff+" more bytes than the indicated box data size, seeking backwards");
if (box.size !== 0) stream.seek(box.start+box.size);
}

If you have a sample file, I could check what's wrong.

@DenizUgur DenizUgur changed the title fix: Build ESDS box missing data property #341 fix: Build ESDS box missing data property Nov 2, 2024
@hughfenghen
Copy link
Author

hughfenghen commented Nov 2, 2024

If the ESDS box is missing the data field, refer to the following code:

this.size = (this.data ? this.data.length : 0);
this.writeHeader(stream);
if (this.data) {
  stream.writeUint8Array(this.data);
}

The box size value will be set to 0, and the raw data will not be written to the output MP4 file. This means the output MP4 file will lack the ESDS box, causing some video players (such as Windows Media Player) to lose audio when playing the file.

Sry, my previous response was ambiguous, mp4box.js does not report an error when parsing MP4 files that lack an ESDS box.

By 'parsing error,' I mean that certain players encounter issues, such as failing to output sound properly, when playing MP4 files generated by mp4box.js that lack an ESDS box.

@DenizUgur
Copy link
Collaborator

Okay I understand you now. The reason I'm hesitant to merge this is that, this is probably not limited to esds and the fix should be more general. I have to check it more thoroughly.

@DenizUgur
Copy link
Collaborator

I believe the decision not to include the data for esds and other boxes was to avoid holding references to them in memory. When parsing boxes, you rarely need the raw data.

The correct approach is to write a box writer for esds. I've already refactored the code a bit; all that remains is to complete src/writing/esds.js.

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.

Build ESDS box missing data property
2 participants