Closed Bug 1368489 Opened 5 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
55 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: opensource.publicocean0, Assigned: bechen)

References

Details

(Keywords: compat)

Attachments

(2 files)

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 
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
(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: nobody → bechen
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)
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)
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)
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)
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
it is sufficient data?
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
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 ....
anyway i can skip the problem creating a indipendent div for every cue.
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
(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
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,...)?
(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.
Summary: cue onexit → The event order of onexit and onenter with the same timestamp.
OK , so texttrack in mse is not yet supported. My way is right for now :)
Keywords: compat
Priority: -- → P1
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 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+
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 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.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/32b32fb358c2
https://hg.mozilla.org/mozilla-central/rev/43e5dfa7b4a5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1372715
You need to log in before you can comment on or make changes to this bug.