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

Text Management refactoring #1844

Merged

Conversation

jeremco
Copy link
Contributor

@jeremco jeremco commented Mar 28, 2017

Hi

This pull request contains some refactoring to have a better comprehension on how subtitles and captions are working on dash.js.
One new text controller is added to manage tracks. The way source buffer controller is created is hidden by a text buffer controller proxy. The text source buffer is dedicated to manage and create cues.
The code relative to text is then centralized and is more easy to understand. Each 'class' has now its own responsability

Jérémie

@dsparacio
Copy link
Contributor

Not sure what this is solving? I get that the code may not be ideal but this CR creates a ton of code climates alerts. Have you tested this with ALL types of captions. Embedded 608/708 fragmented ISo encaplustaed, VTT and TTML Sidecar?

@TobbeEdgeware
Copy link

At first glance, this looks good. I'll try to go through this in the next few days to understand how it works and check if our various subtitle samples are still working properly.

@jeremco
Copy link
Contributor Author

jeremco commented Apr 5, 2017

@AkamaiDASH : The idea is to centralized treatments for text, because in actual code, there is some specifics text treatments made in MediaPlayer, in BufferController and so on
If the treatment is centralized, it will help to find any problems about text.
The idea is although to have only one 'responsibility' for classes. Because, here we have a text source buffer that :

  • Manage track selection
  • Display HTML for embedded captions
  • And fills browser text track queue.
    It results in a big file, that is not very comprehensive at first glance.

I have test on all text streams of the dash-if sample page.

@dsparacio
Copy link
Contributor

We are testing with this PR a bit but should make it into the release. Looks good at high level.

@dsparacio
Copy link
Contributor

@jeremco Thanks for the details and thanks for the commit. Looks nice.

@TobbeEdgeware
Copy link

I checked the various subtitle samples included in the reference player content list.
I found a bug when switching between content that I tracked down to the resetEmbedded function in TextSourceBuffer. Since the textTracks is set to null in the abort() function, we must have a test in the resetEmbedded function.

function resetEmbedded() {
    eventBus.off(Events.VIDEO_CHUNK_RECEIVED, onVideoChunkReceived, this);
    if (textTracks !== null) {
        textTracks.deleteAllTextTracks();
    }

I made a PR to this PR for this.

After the fix, all subtitles work. There is still an issue with the mixed Subtitle embedded-captioning sample where the subtitles is strangely formatted in the reference player, but that is nothing new.

I'll look around a bit more, but in general, I think this looks good for inclusion in the release.

@jeremco
Copy link
Contributor Author

jeremco commented Apr 6, 2017

Thanks for the PR. I have accepted your modification

@jeremco jeremco force-pushed the SubRefactoring branch 2 times, most recently from 36e7d77 to 120d51d Compare April 10, 2017 07:34
jeremco and others added 8 commits April 10, 2017 09:40
- Add a text directory
- Add a proxy class for text buffer (it will be used for each type of text)
- Extract HTML render for embedded text, from TextSourceBuffer
- Add a Text controller, to replace links to text source buffer
- Use proxy TextBufferController to instanciate fragmentedText buffer controller
- Configuration of text controller
- Track management is now done by the controller and not anymore by the text source buffer
- Send a VIDEO_CHUNK_RECEIVED event, catch by text source buffer. It's up to the text source buffer to check for embedded text
- Fix a pb when first displayed stream contains embedded streams, displaying another stream will then display embedded subtitles
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

4 participants