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

RESOLVED FIXED in Firefox 56

Status

()

Product:
Core
Component:
Audio/Video: Playback
Importance:
P1
normal
Status:
RESOLVED FIXED
Reported:
a year ago
Modified:
a year ago

People

(Reporter: opensource.publicocean0, Assigned: bechen)

Tracking

({compat})

Version:
55 Branch
Target:
mozilla56
Keywords:
compat
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

Bug 1368489 - Add testcase to testing the order for the events when cue1's endTime is equal to cue2's stratTime.
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1368489 - Fixing TimeMarchesOn step 13, sort the tasks by "text track cue order". .
59 bytes, text/x-review-board-request
rillian
: review+
Details
(Reporter)

Description

•
a year ago 
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
(Reporter)

Comment 1

•
a year ago 
(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
Component: Untriaged → Audio/Video
Product: Firefox → Core
Component: Audio/Video → Audio/Video: Playback
 (Assignee)

Updated

•
a year ago
Assignee: nobody → bechen
 (Assignee)

Comment 2

•
a year ago 
Can you provide more information about how to reproduce it? Or more detail about your testing data?
Such as cue.startTime cue.endTime.
Flags: needinfo?(opensource.publicocean0)
 (Reporter)

Comment 3

•
a year ago 
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);
				   }	
				}
Flags: needinfo?(opensource.publicocean0)
 (Assignee)

Comment 4

•
a year ago 
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.
Flags: needinfo?(opensource.publicocean0)
 (Reporter)

Comment 5

•
a year ago 
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
Flags: needinfo?(opensource.publicocean0)
 (Reporter)

Comment 6

•
a year ago 
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
 (Reporter)

Comment 7

•
a year ago 
it is sufficient data?
 (Reporter)

Comment 8

•
a year ago 
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
 (Reporter)

Comment 9

•
a year ago 
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 ....
 (Reporter)

Comment 10

•
a year ago 
anyway i can skip the problem creating a indipendent div for every cue.
 (Reporter)

Comment 11

•
a year ago 
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
 (Assignee)

Comment 12

•
a year ago 
(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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
 (Reporter)

Comment 13

•
a year ago 
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,...)?
 (Assignee)

Comment 14

•
a year ago 
(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

•
a year ago 
As far as I know from jya, Firefox doesn't support texttrack in MSE.
 (Assignee)

Updated

•
a year ago
Summary: cue onexit → The event order of onexit and onenter with the same timestamp.
 (Reporter)

Comment 16

•
a year ago 
OK , so texttrack in mse is not yet supported. My way is right for now :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Keywords: compat
Priority: -- → P1

Comment 19

•
a year ago
mozreview-review 
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.
Attachment #8874346 - Flags: review?(giles) → review+

Comment 20

•
a year ago
mozreview-review 
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?
Attachment #8874347 - Flags: review?(giles) → review+
 (Assignee)

Comment 21

•
a year ago
mozreview-review 
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.
 (Assignee)

Comment 22

•
a year ago
mozreview-review 
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

•
a year ago
Keywords: checkin-needed

Comment 27

•
a year ago 
Pushed by ryanvm@gmail.com:
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.
Keywords: checkin-needed

Comment 28

•
a year ago
bugherder 
https://hg.mozilla.org/mozilla-central/rev/32b32fb358c2
https://hg.mozilla.org/mozilla-central/rev/43e5dfa7b4a5
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
 (Assignee)

Updated

•
a year ago
Depends on: 1372715
You need to log in before you can comment on or make changes to this bug.