Created an attachment (id=1060849) DEFECT DESCRIPTION: The play interface display not right. REPRODUCING PROCEDURES: Play a video with 2s[it will be more clear when the video is shorter] from Filemanger or Video,after finish the playing--KO EXPECTED BEHAVIOUR: The play bar should be from begining. ASSOCIATE SPECIFICATION: TEST PLAN REFERENCE: TOOLS AND PLATFORMS USED: USER IMPACT: REPRODUCING RATE: For FT PR, Please list reference mobile's behavior:2611692(61692). ["dom.player.currentTime" is not 0 after video play end]

Hi Norry, qawanted for Woodduck 2.0M and Flame 2.0/2.1/2.2. Thanks!

The bug description is very confusing for this issue, but I have inferred from the expected results that "The play interface display not right." is referring to an issue where the timer does not fully reset when the video ends. I also checked this issue with a 2s video and a 30s video in case 2s was too short as I wasn't quite sure if there was a minimum length to view this issue. If any of this is wrong, my results are invalid. If the reporter could clarify that would be helpful. I did NOT see a bug on 2.2 Flame, 2.1 Flame, or 2.0 Flame when videos ended. The play timer fully reset on both videos checked. Environmental Variables: Device: Flame 2.2 BuildID: 20141217035735 Gaia: d22dfece04fc00457e8369c660c11f945b088d2f Gecko: cb8ad2251c09 Gonk: Could not pull gonk. Did you shallow Flash? Version: 37.0a1 (2.2) Firmware Version: v188-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 Environmental Variables: Device: Flame 2.1 BuildID: 20141216151310 Gaia: 14315733e2d265a42f9ab02d1aba191789870f70 Gecko: ddecea83ce6e Gonk: Could not pull gonk. Did you shallow Flash? Version: 34.0 (2.1) Firmware Version: v188-1 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 Environmental Variables: Device: Flame 2.0 BuildID: 20141216151108 Gaia: d04710d5d643eeff5a6493aef92a1af672a2769c Gecko: 4d62570b77e4 Gonk: Could not pull gonk. Did you shallow Flash? Version: 32.0 (2.0) Firmware Version: v188-1 User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Reporter - can you review comment 2 and verify the tester is understanding the expected results correctly?

The slider can't back to zero after the video play end. (video length = 2s) because /gaia/app/video/js/video.js function updateVideoControlSlider() { var percent = (dom.player.currentTime / dom.player.duration) * 100; ... dom.elapsedTime.style.width = percent; ... } when it play end the "dom.player.currentTime" always not 0. So,it caused that we can't set slider back to 0 position. can you help us to inspect it?

Hi Gary, Could you please help to check the problem? Thanks!

Hi Blake, When we play a video to the end and stop, gaia's video app will set video currentTime to zero. But somehow currentTime could be non zero value after that and leads to progress bar position incorrect. Can you help on this problem? thanks!

After checking with chens, this problem only happens on Woodduck, not found on Flame. It may be a platform specific problem.

As comment 7 said, the bug can be repro on woodduck, and not found on Flame.

(In reply to comment #7) > Comment from Mozilla:After checking with chens, this problem only happens on > Woodduck, not found on Flame. > It may be a platform specific problem. > can we make some change in gaia to avoid this problem?

IIRC, the entry point at the Gecko's side to get current time is [1] [1]http://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1310

(In reply to comment #10) > Comment from Mozilla:IIRC, the entry point at the Gecko's side to get current > time is [1] > > [1]http://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1310 > when play end, HTMLMediaElement::CurrentTime() is not 0.

Created an attachment (id=1080122) our HTMLMediaElement.cpp

Created an attachment (id=1080122) our HTMLMediaElement.cpp

Created an attachment (id=1080122) our HTMLMediaElement.cpp

Hi Kilik, Would you have time to check this bug?

(In reply to Blake Wu [:bwu][:blakewu] from comment #15) > Hi Kilik, > Would you have time to check this bug? Sure, I'll check with chens, and check if gecko get/set correct current time.

Hi Kilik, Just NI you as reminder. Thank you for the help!

any updates?

MediaDecoderStateMachine.newCurrentTime will be updated according to mAudioStartTime in the attached MediaDecorderStateMachine.cpp#2015, and audio time of the first AudioData in AudioQueue seems not to be 0 (the value varies at each time user record). Need to check why the the audio start time varies.

I'll check video recorded from flame to verify the timestamp, first

Hi Kilik, Can partner try patch from your comment 20? Thanks!

I recorded 2 videos(1 by Woodduck, 1 by Flame) and check their first audio data timestamp. I found that the first AudioData->mTime in AudioQueue() (237166 us, recoded by Woodduck) is quite large than the one (21333 us, by flame). 1. Might need media recorder related engineers to investigate. 2. We could implementation a temporary solution like current v2.2, PushFront an AudioData into AudioQueue, and after seek completed, drop audio up to the seekTarget, and set the current timestamp according to the AudioData pushed previously.

Using flame to record the woodduck recording procedure. The first audio timestamp of AudioData is 21333 us. (Recorded by flame) But it's size is larger than upload limit(10mb), so it's compressed (please do NOT use this file for debugging, the timestamp is re-encoded, I just want to demo the woodduck recording procedure). You could also see that the woodduck recording start time 0:00 displayed is much later after I pressed record button.

Hi Blake: per comment#23 would you help investigate from media record side as well? Thanks!

Hi Kilik: in the mean time would you also work on a patch for partner to verify, like mentioned in comment#23? Thank you!

(In reply to Wesly Huang from comment #25) > Hi Blake: per comment#23 would you help investigate from media record side > as well? Thanks! Sorry. I am no expert in recording part. But I have created a follow-up bug (bug 1118090) for "Component: Video/Audio: Recording"

(In reply to Wesly Huang from comment #26) > Hi Kilik: in the mean time would you also work on a patch for partner to > verify, like mentioned in comment#23? Thank you! Sure, I'm working on the patch. Though the code base is quite different between 2.0m and current 2.2, but I don't think it will be a big issue.

Following the logic flow from 2.2, pushing an AudioData in the front of AudioQueuea after accurate seek would fix this issue. But, the slide won't still be in correct position while pressing SeekToBegin/SeekToEnd button. After deliberation, it seems to be much simple and harmless to fix this issue by adjusting the newCurrentTime only. DecodeStateMachine still could play silence during mStartTime and mAudioStartTime. Blake, do you have any suggestion about this solution ?

Comment on attachment 8544973 [details] [diff] [review] Solution_2_Adjust_CurrentTimeForSlide.patch Review of attachment 8544973 [details] [diff] [review]: ----------------------------------------------------------------- Per discussion offline, we should use the min(videoTime, audioTime) as newCurrentTime after seeking since the timestamp of first audio frame could be larger than video's which makes playback position incorrect.

(In reply to Blake Wu [:bwu][:blakewu] from comment #30) > Comment on attachment 8544973 [details] [diff] [review] > Solution_2_Adjust_CurrentTimeForSlide.patch > > Review of attachment 8544973 [details] [diff] [review]: > ----------------------------------------------------------------- > > Per discussion offline, > we should use the min(videoTime, audioTime) as newCurrentTime after seeking > since the timestamp of first audio frame could be larger than video's which > makes playback position incorrect. JW, How do you think?

The closest one instead of the smaller one since we might seek to the end instead of the beginning.

(In reply to JW Wang [:jwwang] from comment #32) > The closest one instead of the smaller one since we might seek to the end > instead of the beginning. [1]Should already consider this case!? [1]http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp?from=MediaDecoderStateMachine.cpp#2439

(In reply to Blake Wu [:bwu][:blakewu] from comment #33) > (In reply to JW Wang [:jwwang] from comment #32) > > The closest one instead of the smaller one since we might seek to the end > > instead of the beginning. > [1]Should already consider this case!? > > [1]http://dxr.mozilla.org/mozilla-central/source/dom/media/ > MediaDecoderStateMachine.cpp?from=MediaDecoderStateMachine.cpp#2439 I mean a seek target where |audio end time| <<< |seek target| < |video end time|.

Comment on attachment 8544973 [details] [diff] [review] Solution_2_Adjust_CurrentTimeForSlide.patch Review of attachment 8544973 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaDecoderStateMachine.cpp @@ +2013,5 @@ > + // better UX, it's not appropriate to set mAudioStartTime to > + // newCurrentTime directly. See Bug 1112438. > + mAudioStartTime = audio ? audio->mTime : seekTime; > + newCurrentTime = (audio && seekTime >= audio->mTime) ? > + audio->mTime : seekTime; Since we're adjusting newCurrentTime in audio-based, and supplemented by video. Considering the following cases ... // ST:start time, ET:end time seek seek |audio ST| < |video ST| <<< ... <<< |audio ET| < |video ET| |audio ST| < |video ST| <<< ... <<< |video ET| < |audio ET| |video ST| < |audio ST| <<< ... <<< |audio ET| < |video ET| |video ST| < |audio ST| <<< ... <<< |video ET| < |audio ET| I think the following check would be enough for newCurrentTime. +if (audio && audio->mTime <= seekTime && seekTime < audio->GetEndTime()) { + newCurrentTime = audio->mTime; +} else { + newCurrentTime = video ? video->mTime : seekTime; +} JW, what do you think ?

I think we use the media time (audio or video) instead of seekTime to show user what is exact playback position mapping to the content. That will be more consistent with what user see on the screen.

Comment on attachment 8544973 [details] [diff] [review] Solution_2_Adjust_CurrentTimeForSlide.patch Review of attachment 8544973 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaDecoderStateMachine.cpp @@ +2011,5 @@ > + // The 1st audio->mTime may be quite larger than 1st video->mTime. > + // While seeking to a position in front of the 1st AudioData, for > + // better UX, it's not appropriate to set mAudioStartTime to > + // newCurrentTime directly. See Bug 1112438. > + mAudioStartTime = audio ? audio->mTime : seekTime; I fail to see how it helps UX by not setting mAudioStartTime to newCurrentTime. See [1] where mStartTime could be a time far ahead of mAudioStartTime. It also leads to inconsistency between playing-from-0 and seeking-to-0-and-play. [1] https://hg.mozilla.org/mozilla-central/annotate/70de2960aa87/dom/media/MediaDecoderStateMachine.cpp#l3204 @@ +2013,5 @@ > + // better UX, it's not appropriate to set mAudioStartTime to > + // newCurrentTime directly. See Bug 1112438. > + mAudioStartTime = audio ? audio->mTime : seekTime; > + newCurrentTime = (audio && seekTime >= audio->mTime) ? > + audio->mTime : seekTime; The logic in your comment is differet from that in the patch. I think the code in your commment is right.

(In reply to JW Wang [:jwwang] from comment #37) > Comment on attachment 8544973 [details] [diff] [review] > Solution_2_Adjust_CurrentTimeForSlide.patch > > Review of attachment 8544973 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/MediaDecoderStateMachine.cpp > @@ +2011,5 @@ > > + // The 1st audio->mTime may be quite larger than 1st video->mTime. > > + // While seeking to a position in front of the 1st AudioData, for > > + // better UX, it's not appropriate to set mAudioStartTime to > > + // newCurrentTime directly. See Bug 1112438. > > + mAudioStartTime = audio ? audio->mTime : seekTime; > > I fail to see how it helps UX by not setting mAudioStartTime to > newCurrentTime. See [1] where mStartTime could be a time far ahead of > mAudioStartTime. It also leads to inconsistency between playing-from-0 and > seeking-to-0-and-play. The UI slide position is based on |mCurrentFrameTime| updated by the call |UpdatePlaybackPositionInternal|. And it'll be updated frequently while seeking. mStartTime is set according to min(mAudioStartTime, mVideoStartTime)[2] after first A/V frame decoded(The behavior is consistent in 2.0 & 2.2). I think the original code logic is already inconsisent... => Play-from-0, set |mAudioStartTime = mStartTime|, the mStartTime is 0 in this case. But the first AudioData->mTime in AudioQueue is not 0. Though the playback is still fine, because we played silence in the beginning. => Seek-to-0-and-play, set |mAudioStartTime| to the first AudioData->mTime(which is not 0) in AudioQueue, and lead to mCurrentFrameTime = mAudioStartTime. That's why the UI slide is in incorrect position. [2] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderReader.cpp#173 That's why I'd like to fix this via loosing the relationship between mCurrentFrameTime and mAudioStartTime by adding more precise condition check. Further, If we'd like to make "Play-from-0" and "Seek-to-0-and-play" consistent, I think we should also set mAudioStartTime to 0 in the call |DecodeSeek| if |seekTime < AudioData->mTime| or set mAudioStartTime to seekTime if |seekTime > AudioData->mTime + AudioData->GetEndTime()|. Does it make sense ? > > [1] > https://hg.mozilla.org/mozilla-central/annotate/70de2960aa87/dom/media/ > MediaDecoderStateMachine.cpp#l3204 > > @@ +2013,5 @@ > > + // better UX, it's not appropriate to set mAudioStartTime to > > + // newCurrentTime directly. See Bug 1112438. > > + mAudioStartTime = audio ? audio->mTime : seekTime; > > + newCurrentTime = (audio && seekTime >= audio->mTime) ? > > + audio->mTime : seekTime; > > The logic in your comment is differet from that in the patch. I think the > code in your commment is right. Ah, I'm sorry, I didn't upload a new patch, I just directly modified the code with better logic in comment to demo concept. Thanks for your feedback :)

Add more precise check.

Hi Kilik, Should we ask partner to try patch per your comment 39? Can you also take this bug? Thanks!

(In reply to Josh Cheng [:josh] OOO from 1/5-1/10 from comment #40) > Hi Kilik, > Should we ask partner to try patch per your comment 39? > Can you also take this bug? > Thanks! I just pushed the patch to try server to check this modification could pass all testing, and yes, they could try this patch to see the result on their device. Once the try result from server has done, I'd ask for review.

I have merged "Solution_2_Adjust_CurrentTimeForSlide_v2.patch" & already send a build to tester. I am waiting for the result. but I try it on my phone, it's work fine. thanks a lot!

Hi Kilik, Could you please raise review process? Thanks!

Comment on attachment 8545846 [details] [diff] [review] Solution_2_Adjust_CurrentTimeForSlide_v2.patch Review of attachment 8545846 [details] [diff] [review]: ----------------------------------------------------------------- Hi Chris, Could you please review this patch ? To summarize, The 1st AudioData timestamp in AudioQueue from the clip recorded by Woodduck is always quite large than VideoData timestamp. And by the logic in in v2.0 |DecodeSeek()|, when seek-to-0-and-play, the slide position will be updated to AudioData timestamp. For a temporary solution, I think it's better to add precise check in addition, to check if the seekTime is ahead of the 1st AudioData timestamp or not. like the discussion from Comment 29 to Comment 38.

Two try results, https://treeherder.mozilla.org/#/jobs?repo=try&revision=e614bb7f0ba9 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef1a5dc4e696

Comment on attachment 8545846 [details] [diff] [review] Solution_2_Adjust_CurrentTimeForSlide_v2.patch Review of attachment 8545846 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaDecoderStateMachine.cpp @@ +2008,4 @@ > newCurrentTime = mAudioStartTime = seekTime; > } else if (HasAudio()) { > AudioData* audio = AudioQueue().PeekFront(); > + mAudioStartTime = audio ? audio->mTime : seekTime; DecodeToTarget() is supposed to prune audio samples at the start of the audio queue so that the start of the first AudioData lies exactly at the seek target: https://github.com/mozilla/gecko-dev/blob/b2g32_v2_0/content/media/MediaDecoderReader.cpp#L308 Is that not happening here? If not, why not?

When seek-to-0-and-play, code would break @ https://github.com/mozilla/gecko-dev/blob/b2g32_v2_0/content/media/MediaDecoderReader.cpp#L305. Here, the startFrame.value() is the first AudioData->mTime in the AudioQueue() which is quite large(237167) than targetFrame.value() which is 0. But I checked the auiod frame information via ffprobe, and I got the result like the following, =============================== 1st AudioFrame [FRAME] media_type=audio key_frame=1 pkt_pts=0 pkt_pts_time=0.000000 pkt_dts=0 pkt_dts_time=0.000000 pkt_duration=11384 pkt_duration_time=0.237167 pkt_pos=552725 pkt_size=341 sample_fmt=fltp nb_samples=1024 channels=2 channel_layout=stereo [/FRAME] ======================= 2nd AudioFrame [FRAME] media_type=audio key_frame=1 pkt_pts=11384 pkt_pts_time=0.237167 pkt_dts=11384 pkt_dts_time=0.237167 pkt_duration=1024 pkt_duration_time=0.021333 pkt_pos=553066 pkt_size=341 sample_fmt=fltp nb_samples=1024 channels=2 channel_layout=stereo [/FRAME] There's a big gap between these 2 frames, and I checked |MediaOmxReader::DecodeAudioData()| @ https://github.com/mozilla/gecko-dev/blob/b2g32_v2_0/content/media/omx/MediaOmxReader.cpp#L359, the first frame won't be pushed into AudioQueue because |source.mSize == 0|

Do we need something like this on mozilla-central as well?

I didn't see large gaps between audio frames in clips recorded by Flame (based on mozilla-central), and the AudioData in clips recorded by Woodduck would be pruned on Flame, then AudioSink::AudioLoop would play silence for it. But I think the logic like this patch should be also good to mozilla-central.

Add r=cpearce. in comment. Carry r+ from cpearce

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #50) > But I think the logic like this patch should be also good to mozilla-central. In that case, can you please attach a patch for m-c that can be landed first? In general, we like to land patches on trunk before the release branches.

Hi Chris, I created another patch for trunk, code logic is the same as the one for 2.0, just did this modification inside different function call. Do I need to set another review for it ? Or I can just carry the review result ? Here's the try result for trunk. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fc77d341f91

Yes we should re-review. Can you upload a file somewhere (either to this bug, or drop-box etc) on which this bug reproduces? I'd like to test it myself. Thanks!

Hello Chris, This is the sample clip recorded by [Woodduck][2.0] When seek-to-0-and-play, DecodeSeek(0) is called, but first AudioData source.mSize == 0, second AudioData (with timestamp 377958) is pushed into the front of AudioQueue. But in [Flame][2.2], |MediaOMXReader::DecodeAudioData()| would push a AudioData(with 0 pts, discontinuity=1) in the front of AudioQueue. And this AudioData would be checked in |MediaDecoderStateMachine::DropAudioUpToSeekTarget()|, and finally a new AudioData(with mCurrentSeekTarget.mTime=0) is pushed to the front. That's a reason why this phenomenon doesn't happen in [2.2].

Hi Chris, Could you please provide feedback per comment 55? Thanks!

Comment on attachment 8552968 [details] [diff] [review] [TRUNK] Adjust current time for slide in a more precise condition after seek.patch Review of attachment 8552968 [details] [diff] [review]: ----------------------------------------------------------------- r+. Sorry for the delay, I've been prioritizing MSE.

(In reply to Chris Pearce (:cpearce) from comment #57) > Comment on attachment 8552968 [details] [diff] [review] > [TRUNK] Adjust current time for slide in a more precise condition after > seek.patch > > Review of attachment 8552968 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+. > > Sorry for the delay, I've been prioritizing MSE. Thanks Chris : )

Rebase to 02/05/2015, carry r+ from cpearce. try result, https://treeherder.mozilla.org/#/jobs?repo=try&revision=b05c284464ed https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f014c67912b

Rebase to 02/05/2015, carry r+ from cpearce. try result for trunk, https://treeherder.mozilla.org/#/jobs?repo=try&revision=766e6c996e7a

Backed out for timeouts in test_played.html. https://hg.mozilla.org/integration/b2g-inbound/rev/57731c479755 https://treeherder.mozilla.org/logviewer.html#?job_id=1296234&repo=b2g-inbound

OSX asserts/crashes too. https://treeherder.mozilla.org/logviewer.html#?job_id=1295304&repo=b2g-inbound

Kilik, could you please take a look at the timeouts and asserts per comment 62 and comment 63?

Sure, I'm checking this now, and will remove the ni? flag once I have an update.

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #65) > Sure, I'm checking this now, and will remove the ni? flag once I have an > update. The mochitest(12) failed due to hitting assertion for this file "gizmo.mp4" in test-3 of test_played.html. ========================= 07:57:06 INFO - [996] WARNING: Decoder=11e890920 Audio not synced after seek, maybe a poorly muxed file?: file /builds/slave/b2g-in-osx64-d-000000000000000/build/src/dom/media/MediaDecoderStateMachine.cpp, line 3260 07:57:06 INFO - Assertion failure: mAudioStartTime == GetMediaTime(), at /builds/slave/b2g-in-osx64-d-000000000000000/build/src/dom/media/MediaDecoderStateMachine.cpp:2026 ========================= It's a test regarding seek-to-2nd-half-of-the-file-and-play case. And in |MediaDecoderStateMachine::SeekCompleted|, we got seekTime = 2794666 (total duration = 5589333), mAudioStartTime = 3008000 (The first AudioData in AudioQueue), mCurrentFrameTime = 2794666 (The first VideoData in VideQueue. By the logic of the uploaded patch, when seekTime < mAudioStartTime, we will check if we can set mCurrentFrameTime according to VideoData). So, when it starts playing, then |StartAudioThread| is called, see https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#2026, (GetMediaTime(2794666) = mStartTime(0)+mCurrentFrameTime(2794666)) != mAudioStartTime(3008000) ... crashed !! But why the first TimeStamp in AudioQueue is not smaller than(or equal to) seekTime ? That's because of this logic here, see https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaOmxReader.cpp#552, we set mAudioSeekTimeUs(3000000) according to the keyframe VideoData, so that the TimeStamp of first AudioData found here is 3008000. To summarize these issues in simpler figure, ======================================================= V : 1st video data in VideoQueue, A : 1st audio data in AudioQueue S : Seek position. Woodduck's problem, time stamp V(0)------A(2xxxxxx)---------> S(0) Mochitest issue, ------------V(3000000)------A(3008000)-------> S(2794666) I'll refine the logic in patch. I think we should NOT set mAudioStartTime = audio->mTime directly if audio exists, see https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#2553, we should also check if seekTime falls between audio->mTime and audio->GetEndTime(), and we should still keep mCurrentFrameTime = mAudioStartTime, and if mAudioStartTime lies before actual 1st audio data timestamp, silence packet shall be injected.

Hello Chris P, Would be please help review these patches again ? I've done a little modification based on Comment 66. (with more critical condition check) Here's the try result for trunk. https://treeherder.mozilla.org/#/jobs?repo=try&revision=95a5e819a86b Thanks : )

This is for 2.0, and I'll post try result once it's accomplished. Thanks.

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #68) > Created attachment 8562015 [details] [diff] [review] > [2.0] Update currentFrameTime & mAudioStartTime with more critical condition > > This is for 2.0, and I'll post try result once it's accomplished. > Thanks. Try result for [2.0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=06d091eab215 Mochitest12 failed even without modification. - B2GRunner TEST-UNEXPECTED-FAIL | Shutdown | application timed out after 450.0 seconds with no output

Comment on attachment 8562014 [details] [diff] [review] [Trunk] Update currentFrameTime & mAudioStartTime with more critical condition Review of attachment 8562014 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +2549,5 @@ > if (seekTime == mEndTime) { > newCurrentTime = mAudioStartTime = seekTime; > } else if (HasAudio()) { > AudioData* audio = AudioQueue().PeekFront(); > + // Though we adjust the newCurrentTime in audio-based, and supplemented This logic is quite complicated. Can it be simplified to something equivalent to: newCurrentTime = min(audio->mTime, video->mTime, seekTime) ?? Maybe: int64_t videoStart = video ? video->mTime : seekTime; int64_t audioStart = audio ? audio->mTime : seekTime; newCurrentTime = min(min(audioStart, videoStart)), seekTime); Does that work?

(In reply to Chris Pearce (:cpearce) from comment #70) > Comment on attachment 8562014 [details] [diff] [review] > [Trunk] Update currentFrameTime & mAudioStartTime with more critical > condition > > Review of attachment 8562014 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +2549,5 @@ > > if (seekTime == mEndTime) { > > newCurrentTime = mAudioStartTime = seekTime; > > } else if (HasAudio()) { > > AudioData* audio = AudioQueue().PeekFront(); > > + // Though we adjust the newCurrentTime in audio-based, and supplemented > > This logic is quite complicated. Can it be simplified to something > equivalent to: > > newCurrentTime = min(audio->mTime, video->mTime, seekTime) > > ?? > > Maybe: > > int64_t videoStart = video ? video->mTime : seekTime; > int64_t audioStart = audio ? audio->mTime : seekTime; > newCurrentTime = min(min(audioStart, videoStart)), seekTime); > > Does that work? Hello Chris, Yeah, this logic is more simple than I've thought originally. I've tested this code on Woodduck and Mochitest-remote on trunk, yes it works. To be more precise, mAudioStartTime should also be updated to avoid assertion while starting audio thread. Like: newCurrentTime = mAudioStartTime = min(min(audioStart, videoStart), seekTime); Thanks for reviewing, and I'll attach new patch soon, : )

Simplified the logic based on Comment 70.

Simplified logic based on Comment 70.

Try result for trunk, https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfb13c8adc47 Try result for 2.0 https://treeherder.mozilla.org/#/jobs?repo=try&revision=69d57105a97b * B2G ICS Emulator debug - Mochitest12 keeps timeout even try without modification.

Please nominate this for b2g34 and b2g37 approval when you get a chance.

Comment on attachment 8565255 [details] [diff] [review] [Trunk] Update currentFrameTime & mAudioStartTime with more critical condition_v2_rebased NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1112438, imprecise media currentTime and audioStartTime are used. User impact if declined: UI slider may at wrong position when playback badly muxed video(very short) after play end. Testing completed: v2.0 & v3.0 Risk to taking this patch (and alternatives if risky): low, only modified several lines of code inside a isolated logic scope. String or UUID changes made by this patch: none

Requesting QA verification on 2.0M or a nightly 3.0 before uplift to branches.

According to Comment 7 this does not occur on Flame only Woodduck. Sherman, can you please verify this? See Comment 80.

Comment on attachment 8565255 [details] [diff] [review] [Trunk] Update currentFrameTime & mAudioStartTime with more critical condition_v2_rebased Approving the landing, but we should verify this in parallel.

(In reply to KTucker [:KTucker] from comment #81) > According to Comment 7 this does not occur on Flame only Woodduck. Sherman, > can you please verify this? See Comment 80. Correct, this doesn't occur on flame.

Hi Chris, Considering the ReleaseManagement/B2G_Landing [1] I guess I need to get this patch reviewed again for uplifting to v2.1. It is rebased for mozilla-b2g34_v2_1 to apply. (Because architecture between v2.0 & v2.1 changed a lot, and the folder tree structure also changed between v2.1 and v2.2) And for uplifting to mozilla-b2g37_v2_2, Attachment 8565255 [details] [diff] is applicable. Thanks for the help :) [1] https://wiki.mozilla.org/Release_Management/B2G_Landing

Comment on attachment 8578567 [details] [diff] [review] [2.1]_AdjustCurrentTimeForSlide_v4_rebase0317.patch Sorry for the inconvenience caused. I got a+ from Comment 82, but that patch(for trunk) is not applicable on b2g34(v2.1), so I rebased it and got another r+ from cpearce. I'm not quite sure about if a+ could be carried ... so I ask for it again. Also a try run for v2.1. https://treeherder.mozilla.org/#/jobs?repo=try&revision=377893b6db52 Thanks. NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1112438, imprecise media currentTime and audioStartTime are used. User impact if declined: UI slider may at wrong position when playback badly muxed video(very short) after play end. Testing completed: v2.0 & v3.0 Risk to taking this patch (and alternatives if risky): low, only modified several lines of code inside a isolated logic scope. String or UUID changes made by this patch: none

Comment on attachment 8578567 [details] [diff] [review] [2.1]_AdjustCurrentTimeForSlide_v4_rebase0317.patch Let's go with the last approval.

This bug has been verified as pass on latest Flame 2.2&2.5&2.6 and Aries KK v2.5 STR: 1. Launch video. 2. Play a video. 3. Drag the progress bar to the end. Actual result: The slider go back to zero Flame v2.2(pass): Build ID 20151102032501 Gaia Revision 885647d92208fb67574ced44004ab2f29d23cb45 Gaia Date 2015-10-07 13:05:24 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b8b7f4efaa6e Gecko Version 37.0 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20151102.070306 Firmware Date Mon Nov 2 07:03:17 EST 2015 Firmware Version v18D v4 Bootloader L1TC000118D0 FlameKK v2.5 build(pass): Build ID 20151102004502 Gaia Revision 91cac94948094cfdcd00cba5c6483e27e80cb3b0 Gaia Date 2015-10-28 20:32:15 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b6b410d4610da18f5e43750e67ed2c56a0c0f812 Gecko Version 44.0a1 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20151102.042848 Firmware Date Mon Nov 2 04:29:02 EST 2015 Bootloader L1TC000118D0 FlameKK v2.6 (master) build(pass): Build ID 20151102150204 Gaia Revision 7954ff0cbd794a35499a1082bed273598f82ee6f Gaia Date 2015-11-02 17:35:17 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/6275cd9c71b76891f6b6585dabc687bc443ab877 Gecko Version 45.0a1 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20151102.182914 Firmware Date Mon Nov 2 18:29:28 EST 2015 Bootloader L1TC000118D0 Aries KK 2.6 eng(pass): Build ID 20151103000930 Gaia Revision 7954ff0cbd794a35499a1082bed273598f82ee6f Gaia Date 2015-11-02 17:35:17 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/9f69202d82752e093a653a8f15b0274e347db33a Gecko Version 45.0a1 Device Name aries Firmware(Release) 4.4.2 Firmware(Incremental) eng.worker.20151102.233051 Firmware Date Mon Nov 2 23:30:58 UTC 2015 Bootloader s1

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/11b9708fcd9a

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=20809199&repo=mozilla-inbound

(In reply to Carsten Book [:Tomcat] from comment #93) > backed out for test failures like > https://treeherder.mozilla.org/logviewer.html#?job_id=20809199&repo=mozilla- > inbound JYA has provided a better solution for this workaround. The backed-out is fine.