1368489 - The event order of onexit and onenter with the same timestamp.

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[opensource.publicocean0]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce:

In my player i add cues. When a cue event happens
cue.onenter it is ok (startTime= t)
but cue.onexit is executed at endTime==startTime.... so pratcally i can see the subtitles....

I tested the player chrome it works correctly


Actual results:

cue.onexit executed in wrong time

Comment 1

[opensource.publicocean0]

(In reply to opensource.publicocean0 from comment #0)
> User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like
> Gecko) Chrome/55.0.2883.87 Safari/537.36
> 
> Steps to reproduce:
> 
> In my player i add cues. When a cue event happens
> cue.onenter it is ok (startTime= t)
> but cue.onexit is executed at endTime==startTime.... so pratcally i can t see
> the subtitles....
> 
> I tested the player chrome it works correctly
> 
> 
> Actual results:
> 
> cue.onexit executed in wrong time

Comment 2

[Benjamin Chen [:bechen]]

Can you provide more information about how to reproduce it? Or more detail about your testing data?
Such as cue.startTime cue.endTime.

Comment 3

[opensource.publicocean0]

this is the code.... i saw in debug firefox execute onenter/onexit ... log show the visualization period is pratically 0.
endtime=startime 

var t=tracks[c.trackId];
				  if (t && t.target instanceof TextTrack ){
						logger('log',"sourcebuffer#" +c.trackId+" - adding cue ["+c.startTime+'-'+c.endTime+']');
						var cue = new VTTCue(c.startTime, c.endTime,c.text);
						cue.is_rap = c.isRap;
						t.target.addCue(cue);
						(function(t){
						cue.onenter=function(){this.ref=t; processInbandCue(this);};
						cue.onexit=function(){this.ref=t;  removeInbandCue(this);};
					   })(t);
				   }	
				}

Comment 4

[Benjamin Chen [:bechen]]

Sorry I still don't fully understand the issue.

1. What are the exact number for the c.startTime and c.endTime?  Are they very closed?
2. Is the subtitle show/hide normally?

There is a situation that the onexit comes right after onenter. That is missing cue.
For the missing cue case, the subtitle should not be shown on the screen.
exmaple: cue.startTime=1s, cue.endTime=2s, for some reason, the playback is not smooth so the playback time are 0s, 5s, the cue is a missing cue.

Comment 5

[opensource.publicocean0]

subtitle is a div ... .show() ... .hide() using jquery

for the first point the difference might be a second. I will post you the exact values

Comment 6

[opensource.publicocean0]

XPlayer:video Time 39.02229, processing cue for track 4 with mime wvtt [This is cue #39 ]   
XPlayer:video Time 39.02229, exiting cue for track 4

Comment 7

[opensource.publicocean0]

it is sufficient data?

Comment 8

[opensource.publicocean0]

i added details to log ...

XPlayer:video Time 495.032857, processing cue for track 4 with mime type wvtt [period:495-496,content:This is cue #495]   
XPlayer:video Time 495.032857, exiting cue for track 4

Comment 9

[opensource.publicocean0]

i found the error in thi log 
video Time 7.02492, processing cue for track 4 with mime type wvtt [period:7-8,content:This is cue #7
]  
video Time 7.02492, exiting cue for track 4[period:6-7]

the existing cue for the previous period is called after enter


flow might be enter t1 exit t1 enter t2 exit t2
instead is enter t2 exit t1 ....

Comment 10

[opensource.publicocean0]

anyway i can skip the problem creating a indipendent div for every cue.

Comment 11

[opensource.publicocean0]

i solved adding a counter  but i added a few of logic for handling not a encreasing temporal order. 
In the case you consider ok so, i advice to write it in the documentation for developers

Comment 12

[Benjamin Chen [:bechen]]

(In reply to opensource.publicocean0 from comment #9)
> i found the error in thi log 
> video Time 7.02492, processing cue for track 4 with mime type wvtt
> [period:7-8,content:This is cue #7
> ]  
> video Time 7.02492, exiting cue for track 4[period:6-7]
> 
> the existing cue for the previous period is called after enter
> 
> 
> flow might be enter t1 exit t1 enter t2 exit t2
> instead is enter t2 exit t1 ....

I see, it is really a bug, thanks for your finding.

cue1[1s-2s], cue2[2s-3s]
The onexit of c1 should be earlier than c2's onenter

Correct implementation at the step 13 of 
https://html.spec.whatwg.org/multipage/embedded-content.html#time-marches-on
It says "sort them intext track cue order"

We need to modify:
https://searchfox.org/mozilla-central/source/dom/html/TextTrackManager.cpp#540-546

Comment 13

[opensource.publicocean0]

Ok can i ask a clarification anyway? now my player add cues using texttrack object in the browser.
but using MSE ... if i add a segment containing a text track is the same? 
browser is able to recognise the different text tracks in the segment (webvtt,srt,...)?

Comment 14

[Benjamin Chen [:bechen]]

(In reply to opensource.publicocean0 from comment #13)
> Ok can i ask a clarification anyway? now my player add cues using texttrack
> object in the browser.
> but using MSE ... if i add a segment containing a text track is the same? 
> browser is able to recognise the different text tracks in the segment
> (webvtt,srt,...)?

I'm not pretty sure the answer. You could try it.
But I guess Firefox doesn't support texttrack in MSE? (SourceBuffer.textTracks)

Comment 15

[Blake Wu [:bwu][:blakewu]]

As far as I know from jya, Firefox doesn't support texttrack in MSE.

Comment 16

[opensource.publicocean0]

OK , so texttrack in mse is not yet supported. My way is right for now :)

Comment 17

[Benjamin Chen [:bechen]]

Attachment: Bug 1368489 - Add testcase to testing the order for the events when cue1's endTime is equal to cue2's stratTime.

Review commit: https://reviewboard.mozilla.org/r/145706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145706/

Comment 18

[Benjamin Chen [:bechen]]

Attachment: Bug 1368489 - Fixing TimeMarchesOn step 13, sort the tasks by "text track cue order". .

1. Sort by TextTrack. 2. Sort by time. 3. Sort by the order of added to TextTrack.

Review commit: https://reviewboard.mozilla.org/r/145708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145708/

Comment 19

[Ralph Giles (:rillian)]

Comment on attachment 8874346 [details]
Bug 1368489 - Add testcase to testing the order for the events when cue1's endTime is equal to cue2's stratTime.

https://reviewboard.mozilla.org/r/145706/#review149882

::: dom/media/test/test_webvtt_event_same_time.html:24
(Diff revision 1)
> +function runTest() {
> +  info("--- create video ---");
> +  var video = document.createElement("video");
> +  video.src = "seek.webm";
> +  video.autoplay = true;
> +  document.getElementById("content").appendChild(video);

does appendChild + autoplay here race the rest of the script? I can't remember? Might be safer to call video.play() at the end of runTest()?

::: dom/media/test/test_webvtt_event_same_time.html:56
(Diff revision 1)
> +    cue3.onenter = null;
> +    c3enter = true;
> +  }
> +  cue4.onenter = function () {
> +    cue4.onenter = null;
> +    ok(c3enter, "cue3 onenter event before than cue4 onenter");

Good idea to check the insertion order is maintained here.

Comment 20

[Ralph Giles (:rillian)]

Comment on attachment 8874347 [details]
Bug 1368489 - Fixing TimeMarchesOn step 13, sort the tasks by "text track cue order". .

https://reviewboard.mozilla.org/r/145708/#review149892

::: dom/media/TextTrackList.cpp:261
(Diff revision 1)
>  }
>  
> +void
> +TextTrackList::GetArray(nsTArray<RefPtr<TextTrack> >& aTextTracks)
> +{
> +  aTextTracks = nsTArray<RefPtr<TextTrack> >(mTextTracks);

I was worried this was expensive, but I think the reference ctor on nsTArray means we don't end up copying all RefPtr's?

Comment 21

[Benjamin Chen [:bechen]]

Comment on attachment 8874346 [details]
Bug 1368489 - Add testcase to testing the order for the events when cue1's endTime is equal to cue2's stratTime.

https://reviewboard.mozilla.org/r/145706/#review149976

::: dom/media/test/test_webvtt_event_same_time.html:24
(Diff revision 1)
> +function runTest() {
> +  info("--- create video ---");
> +  var video = document.createElement("video");
> +  video.src = "seek.webm";
> +  video.autoplay = true;
> +  document.getElementById("content").appendChild(video);

No, I don't think there is race inside. If there is a race issue, it is definitely a bug we should solve in MediaElement.

Comment 22

[Benjamin Chen [:bechen]]

Comment on attachment 8874347 [details]
Bug 1368489 - Fixing TimeMarchesOn step 13, sort the tasks by "text track cue order". .

https://reviewboard.mozilla.org/r/145708/#review149978

::: dom/media/TextTrackList.cpp:261
(Diff revision 1)
>  }
>  
> +void
> +TextTrackList::GetArray(nsTArray<RefPtr<TextTrack> >& aTextTracks)
> +{
> +  aTextTracks = nsTArray<RefPtr<TextTrack> >(mTextTracks);

I should avoid the copy operation, just use reference.

Comment 23

[Benjamin Chen [:bechen]]

Comment on attachment 8874346 [details]
Bug 1368489 - Add testcase to testing the order for the events when cue1's endTime is equal to cue2's stratTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145706/diff/1-2/

Comment 24

[Benjamin Chen [:bechen]]

Comment on attachment 8874347 [details]
Bug 1368489 - Fixing TimeMarchesOn step 13, sort the tasks by "text track cue order". .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145708/diff/1-2/

Comment 25

[Benjamin Chen [:bechen]]

Comment on attachment 8874346 [details]
Bug 1368489 - Add testcase to testing the order for the events when cue1's endTime is equal to cue2's stratTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145706/diff/2-3/

Comment 26

[Benjamin Chen [:bechen]]

Comment on attachment 8874347 [details]
Bug 1368489 - Fixing TimeMarchesOn step 13, sort the tasks by "text track cue order". .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145708/diff/2-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32b32fb358c2
Add testcase to testing the order for the events when cue1's endTime is equal to cue2's stratTime. r=rillian
https://hg.mozilla.org/integration/autoland/rev/43e5dfa7b4a5
Fixing TimeMarchesOn step 13, sort the tasks by "text track cue order". r=rillian.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32b32fb358c2
https://hg.mozilla.org/mozilla-central/rev/43e5dfa7b4a5