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

allow multiple moofs per segment #1954

Conversation

squarebracket
Copy link
Contributor

I don't expect this PR as it currently is to get accepted, but it's a good place to discuss.

Right now, dash.js only parses out the text information from the first moof in a segment. It would be nice if the text data could be pulled out of all the moofs in a segment.

This PR patches in support for multiple moofs per segment for CEA608 captions.

Is there anywhere else in the code where multiple moofs per segment are rejected, or is it only the text stuff?

@TobbeEdgeware
Copy link

TobbeEdgeware commented May 23, 2017

This is a good addition. Especially for embedded CEA-608 data, since video segments may be partitioned into multiple isobmff fragments.

This could also happen for other subtitles, so we should support that as well.

AFAIK, it is only for text that dash.js need to access the moof box in order to extract the actual text data.

I will look closer at this PR:

@TobbeEdgeware TobbeEdgeware self-requested a review May 23, 2017 06:36
@squarebracket
Copy link
Contributor Author

squarebracket commented May 23, 2017

Which formats are you referring to when you say "other subtitles"?

@squarebracket
Copy link
Contributor Author

Alright, I think I've got it good for all text types now. It might be overkill, but it's at least robust in that it follows what the ISOBMFF spec allows. I've also refactored the extraction of cea608 data to reuse the parsing that's already done earlier in the code, rather than having to parse the MP4 data twice.

If I can figure it out, I'd like to add some tests for text parsing; I feel like mucking around with the MP4 box parsing can easily mess things up.

In the mean time, if you have any media that I should try that's not listed in the "Subtitles and Captions" part of the reference player, please let me know so I can give it a test.

@dsparacio dsparacio merged commit 36baa49 into Dash-Industry-Forum:development Jun 2, 2017
@squarebracket squarebracket deleted the multiple-moofs-per-segment branch June 2, 2017 18:36
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.

None yet

3 participants