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)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: opensource.publicocean0, Assigned: bechen)
References
Details
(Keywords: compat)
Attachments
(2 files)
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•5 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
Updated•5 years ago
|
Component: Untriaged → Audio/Video
Product: Firefox → Core
Updated•5 years ago
|
Component: Audio/Video → Audio/Video: Playback
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → bechen
Assignee | ||
Comment 2•5 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•5 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•5 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•5 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•5 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•5 years ago
|
||
it is sufficient data?
Reporter | ||
Comment 8•5 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•5 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•5 years ago
|
||
anyway i can skip the problem creating a indipendent div for every cue.
Reporter | ||
Comment 11•5 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•5 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•5 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•5 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)
Comment 15•5 years ago
|
||
As far as I know from jya, Firefox doesn't support texttrack in MSE.
Assignee | ||
Updated•5 years ago
|
Summary: cue onexit → The event order of onexit and onenter with the same timestamp.
Reporter | ||
Comment 16•5 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) |
Comment 19•5 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•5 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•5 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•5 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•5 years ago
|
Keywords: checkin-needed
Comment 27•5 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•5 years ago
|
||
bugherder |
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
You need to log in
before you can comment on or make changes to this bug.
Description
•