Fix captions disappear on unbuffered seeks when user activates, deactivates, and reactivates captions #2674

muhammadharis · 2020-06-22T23:23:36Z

What is the bug?
#2671

Why does this bug exist?
When the subtitles are turned off, unloadTextStream() on streaming engine is called, but currentTextStream is never set to null. When the captions are turned back on, a check in player.js sees that the current text stream is not null, so it assumes it exists, and is ready to go. Thus captions are never loaded.

What is the fix?
Clearing (setting to null) the current stream on unloadTextStream ensures the text stream is properly initialized the next time it is called, and in a state so that captions can resume being parsed.

Notes
After further investigation, it seems that the unit tests are written in a way that assumes that the text stream is nulled when unloadTextStream is called. So this fix is definitely something that we assume already happens.

…ject#2648)

…tivating, activating subtitles

…bug-user-activate-and-deactivate

joeyparrish

This looks great. Any chance you could try adding a test for this? Maybe player_unit.js?

muhammadharis · 2020-06-23T16:39:52Z

@joeyparrish While further investigating this issue, I found a more elegant solution, and one that, from the way the code and unit tests are currently written, it seems that we assume already exists. I've described it in the edited summary of the pull request.

Furthermore, I found another bug (which I now fixed) while fixing this issue, where captions show up in 2 different languages simultaneously when turned off, then back on in a different language. This happens because the UI library calls setTextTrackVisibility then selectTextLanguage, which both try to switch the text stream to different languages. However, if we reverse the order in which they are called, then when setTextTrackVisibility is called after selectTextLanguage, it will see that a language is already selected, and won't try to switch the text stream, fixing the issue!

joeyparrish · 2020-06-23T23:21:35Z

lib/media/streaming_engine.js

@@ -251,6 +251,7 @@ shaka.media.StreamingEngine = class {
      this.abortOperations_(state).catch(() => {});
      this.mediaStates_.delete(ContentType.TEXT);
    }
+    this.currentTextStream_ = null;


Oh, wow, that certainly should have been there. :-)

This member is new in v3.0 if I recall correctly.

joeyparrish · 2020-06-23T23:23:07Z

test/player_unit.js

@@ -341,6 +341,19 @@ describe('Player', () => {
        expect(streamingEngine.switchTextStream).not.toHaveBeenCalled();
        expect(streamingEngine.unloadTextStream).not.toHaveBeenCalled();
      });
+
+      it('unloads text stream when captions are turned off', async () => {


It seems that this test only verifies that unloadTextStream() is called, but your fix was to make sure that getCurrentTextStream() returns null after unloadTextStream().

So instead of verifying the call to unloadTextStream() happened (which is an implementation detail), I think you should be verifying the value of getCurrentTextStream() at the end (which is the observable behavior in the rest of the system).

joeyparrish · 2020-06-23T23:25:04Z

ui/text_selection.js

    if (this.player) {  // May have become null while awaiting
      this.player.selectTextLanguage(track.language, track.roles[0]);
    }
+    await this.player.setTextTrackVisibility(true);


Moving the visibility change after selectTextLanguage() removes the need to check for this.player. We know it's non-null at the start of the method, but it may become null in the middle of an async call if someone calls destroy().

But I'm also not entirely sure why you changed this. Can you explain?

Oh, sorry, now I see your comment from before:

I found another bug (which I now fixed) while fixing this issue, where captions show up in 2 different languages simultaneously when turned off, then back on in a different language. This happens because the UI library calls setTextTrackVisibility then selectTextLanguage, which both try to switch the text stream to different languages. However, if we reverse the order in which they are called, then when setTextTrackVisibility is called after selectTextLanguage, it will see that a language is already selected, and won't try to switch the text stream, fixing the issue!

Then please remove the if (this.player) above and leave a comment explaining why these calls need to be in this order. Otherwise, we may change it back some day without realizing we broke anything. (Even better would be an automated test for this, but I would accept a strongly-worded comment in this case.)

joeyparrish

Looks good otherwise. Thanks!

joeyparrish · 2020-06-24T16:21:23Z

test/player_unit.js

+        await player.setTextTrackVisibility(true);
+        await player.load(fakeManifestUri, 0, fakeMimeType);
+        expect(streamingEngine.switchTextStream).toHaveBeenCalled();
+        expect(shaka.test.Util.invokeSpy(streamingEngine.getCurrentTextStream))


The invokeSpy wrapper is a compiler hack since the compiler doesn't see jasmine Spy objects as callable. But in this case, I believe this method is not a Spy, so you should be able to remove the wrapper and call it directly. Same below.

If I'm wrong, as I often am, build/check.py would complain after removing the wrapper.

I just verified, and it seems like there is a Jasmine Spy created for this method in simple_fakes.js (https://github.com/google/shaka-player/blob/master/test/test/util/simple_fakes.js#L103). Consequently, the compiler complains when I remove the wrapper and call the method directly.

Oh, I see. This is player_unit, so streamingEngine is a fake. Now the spy part makes more sense.

But since the fix is in StreamingEngine, testing with a fake StreamingEngine doesn't verify the fix. :-) You could comment out your one-line fix, and this test would still pass.

You'll need to test this in test/media/streaming_engine_unit.js instead.

shaka-bot · 2020-06-24T19:07:06Z

All tests passed!

When the subtitles are turned off, unloadTextStream() on streaming engine is called, but currentTextStream is never set to null. When the captions are turned back on, a check in player.js sees that the current text stream is not null, so it assumes it exists, and is ready to go. Thus captions are never loaded. Clearing (setting to null) the current stream on unloadTextStream ensures the text stream is properly initialized the next time it is called, and in a state so that captions can resume being parsed. After further investigation, it seems that the unit tests are written in a way that assumes that the text stream is nulled when unloadTextStream is called. So this fix is definitely something that we assume already happens. Closes #2671

muhammadharis added 5 commits June 11, 2020 18:42

Created MP4 box parsers to parse data for common box types (shaka-pro…

ffaed64

…ject#2648)

merged in master

e17d2f1

fix: captions disappear during unbuffered seeks when activating, deac…

2fb10c1

…tivating, activating subtitles

Merge branch 'master' of https://github.com/google/shaka-player into …

e9d299b

…bug-user-activate-and-deactivate

removed extra file from unrebased local master

c22d766

muhammadharis mentioned this pull request Jun 22, 2020

Issue when the user activate and deactivate the subtitles. #2671

Closed

joeyparrish requested changes Jun 22, 2020

View reviewed changes

muhammadharis added 2 commits June 23, 2020 12:34

fixed captions disappearing or showing double when turned off then on

9c0db06

removed extraneous null test

57ef3bb

muhammadharis requested a review from joeyparrish June 23, 2020 16:40

minor semantic fix in test description

b677f11

joeyparrish requested changes Jun 23, 2020

View reviewed changes

muhammadharis added 2 commits June 23, 2020 20:36

added descriptive comment and tightened tests as per review

037011a

modified test message

1629ee7

muhammadharis requested a review from joeyparrish June 24, 2020 15:55

joeyparrish requested changes Jun 24, 2020

View reviewed changes

muhammadharis requested a review from joeyparrish June 24, 2020 17:57

muhammadharis added 2 commits June 24, 2020 14:27

added unit test in streaming_engine_unit.js

a1f117a

removed extra setup from test

a8808cd

joeyparrish approved these changes Jun 24, 2020

View reviewed changes

joeyparrish merged commit d53fbae into shaka-project:master Jun 24, 2020

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix captions disappear on unbuffered seeks when user activates, deactivates, and reactivates captions #2674

Fix captions disappear on unbuffered seeks when user activates, deactivates, and reactivates captions #2674

muhammadharis commented Jun 22, 2020 •

edited

Loading

joeyparrish left a comment

muhammadharis commented Jun 23, 2020 •

edited

Loading

joeyparrish Jun 23, 2020

joeyparrish Jun 23, 2020

joeyparrish Jun 23, 2020

joeyparrish Jun 23, 2020

joeyparrish left a comment

joeyparrish Jun 24, 2020

muhammadharis Jun 24, 2020 •

edited

Loading

joeyparrish Jun 24, 2020

shaka-bot commented Jun 24, 2020

Fix captions disappear on unbuffered seeks when user activates, deactivates, and reactivates captions #2674

Fix captions disappear on unbuffered seeks when user activates, deactivates, and reactivates captions #2674

Conversation

muhammadharis commented Jun 22, 2020 • edited Loading

joeyparrish left a comment

Choose a reason for hiding this comment

muhammadharis commented Jun 23, 2020 • edited Loading

joeyparrish Jun 23, 2020

Choose a reason for hiding this comment

joeyparrish Jun 23, 2020

Choose a reason for hiding this comment

joeyparrish Jun 23, 2020

Choose a reason for hiding this comment

joeyparrish Jun 23, 2020

Choose a reason for hiding this comment

joeyparrish left a comment

Choose a reason for hiding this comment

joeyparrish Jun 24, 2020

Choose a reason for hiding this comment

muhammadharis Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

joeyparrish Jun 24, 2020

Choose a reason for hiding this comment

shaka-bot commented Jun 24, 2020

muhammadharis commented Jun 22, 2020 •

edited

Loading

muhammadharis commented Jun 23, 2020 •

edited

Loading

muhammadharis Jun 24, 2020 •

edited

Loading