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

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: opensource.publicocean0, Assigned: bechen)

Tracking

({compat})

55 Branch
mozilla56
compat
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years 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

2 years 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

2 years ago
Assignee: nobody → bechen
(Assignee)

Comment 2

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
it is sufficient data?
(Reporter)

Comment 8

2 years 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

2 years 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

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

Comment 11

2 years 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

2 years 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

2 years 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

2 years 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)
As far as I know from jya, Firefox doesn't support texttrack in MSE.
(Assignee)

Updated

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

Comment 16

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Keywords: checkin-needed

Comment 27

2 years 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

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

Updated

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