Closed Bug 1112438 Opened 5 years ago Closed 5 years ago

[FFOS2.0][Woodduck][Video]Slider doesn't go back to zero after the video play end.

Categories

(Firefox OS Graveyard :: Gaia::Video, defect, P2)

defect

Tracking

(blocking-b2g:2.0M+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.0 unaffected, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S6 (20feb)
blocking-b2g 2.0M+
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: sync-1, Assigned: kikuo)

References

Details

(Keywords: verifyme)

Attachments

(8 files, 8 obsolete files)

123.91 KB, text/x-c++src
Details
101.22 KB, text/x-c++src
Details
555.93 KB, video/3gpp
Details
1.56 MB, video/3gpp
Details
664.29 KB, video/3gpp
Details
1.77 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.74 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.76 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
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!
Flags: needinfo?(fan.luo)
Keywords: qawanted
blocking-b2g: --- → 2.0M?
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Reporter - can you review comment 2 and verify the tester is understanding the expected results correctly?
Flags: needinfo?(jmitchell) → needinfo?(sync-1)
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!
Flags: needinfo?(gchen)
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!
Flags: needinfo?(gchen) → needinfo?(bwu)
Summary: [FFOS2.0][Woodduck][Video]The play video interface display not right. → [FFOS2.0][Woodduck][Video]Slider doesn't go back to zero after the video play end.
After checking with chens, this problem only happens on Woodduck, not found on Flame. 
It may be a platform specific problem.
Flags: needinfo?(bwu)
As comment 7 said, the bug can be repro on woodduck, and not found on Flame.
Flags: needinfo?(fan.luo)
Keywords: qawanted
(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?
Flags: needinfo?(kikuo)
(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.
Flags: needinfo?(kikuo)
Duplicate of this bug: 1114402
Hi Kilik,
Just NI you as reminder. Thank you for the help!
Flags: needinfo?(kikuo)
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
Flags: needinfo?(kikuo)
Hi Kilik,
Can partner try patch from your comment 20?
Thanks!
Flags: needinfo?(kikuo)
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.
Flags: needinfo?(kikuo)
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!
Flags: needinfo?(bwu)
Hi Kilik: in the mean time would you also work on a patch for partner to verify, like mentioned in comment#23? Thank you!
Flags: needinfo?(kikuo)
(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"
Flags: needinfo?(bwu)
(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.
Flags: needinfo?(kikuo)
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 ?
Attachment #8544973 - Flags: feedback?(bwu)
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.
Attachment #8544973 - Flags: feedback?(bwu)
(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?
Flags: needinfo?(jwwang)
The closest one instead of the smaller one since we might seek to the end instead of the beginning.
Flags: needinfo?(jwwang)
(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
Flags: needinfo?(sync-1) → needinfo?(kikuo)
Depends on: 1118090
See Also: → 1118090
(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|.
No longer depends on: 1118090
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 ?
Attachment #8544973 - Flags: feedback?(jwwang)
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.
Attachment #8544973 - Flags: feedback?(jwwang)
(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 :)
Flags: needinfo?(kikuo)
Add more precise check.
Attachment #8544973 - Attachment is obsolete: true
Hi Kilik,
Should we ask partner to try patch per your comment 39? 
Can you also take this bug?
Thanks!
Flags: needinfo?(kikuo)
Duplicate of this bug: 1114404
(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.
Assignee: nobody → kikuo
Flags: needinfo?(kikuo)
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!
Flags: needinfo?(kikuo)
blocking-b2g: 2.0M? → 2.0M+
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.
Attachment #8545846 - Flags: review?(cpearce)
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?
Flags: needinfo?(kikuo)
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|
Flags: needinfo?(kikuo)
Flags: needinfo?(cpearce)
Attachment #8545846 - Flags: review?(cpearce) → review+
Do we need something like this on mozilla-central as well?
Flags: needinfo?(cpearce)
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
Attachment #8545846 - Attachment is obsolete: true
Attachment #8551060 - Flags: review+
Keywords: checkin-needed
(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
Flags: needinfo?(kikuo) → needinfo?(cpearce)
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!
Flags: needinfo?(cpearce)
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].
Flags: needinfo?(cpearce)
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.
Attachment #8552968 - Flags: review+
Flags: needinfo?(cpearce)
(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 for trunk,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=766e6c996e7a
Attachment #8552968 - Attachment is obsolete: true
Attachment #8559639 - Flags: review+
Keywords: checkin-needed
Kilik, could you please take a look at the timeouts and asserts per comment 62 and comment 63?
Flags: needinfo?(kikuo)
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.
Flags: needinfo?(kikuo)
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 : )
Attachment #8559639 - Attachment is obsolete: true
Attachment #8562014 - Flags: review?(cpearce)
This is for 2.0, and I'll post try result once it's accomplished.
Thanks.
Attachment #8559638 - Attachment is obsolete: true
Attachment #8562015 - Flags: review?(cpearce)
Status: NEW → ASSIGNED
(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?
Flags: needinfo?(kikuo)
(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, : )
Flags: needinfo?(kikuo)
Simplified the logic based on Comment 70.
Attachment #8562015 - Attachment is obsolete: true
Attachment #8562015 - Flags: review?(cpearce)
Attachment #8565254 - Flags: review?(cpearce)
Simplified logic based on Comment 70.
Attachment #8562014 - Attachment is obsolete: true
Attachment #8562014 - Flags: review?(cpearce)
Attachment #8565255 - Flags: review?(cpearce)
Attachment #8565254 - Flags: review?(cpearce) → review+
Attachment #8565255 - Flags: review?(cpearce) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b812ca945b2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S6 (20feb)
Please nominate this for b2g34 and b2g37 approval when you get a chance.
Flags: needinfo?(kikuo)
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
Flags: needinfo?(kikuo)
Attachment #8565255 - Flags: approval-mozilla-b2g37?
Attachment #8565255 - Flags: approval-mozilla-b2g34?
Keywords: verifyme
Requesting QA verification on 2.0M or a nightly 3.0 before uplift to branches.
Flags: needinfo?(ktucker)
According to Comment 7 this does not occur on Flame only Woodduck. Sherman, can you please verify this? See Comment 80.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(chens)
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.
Attachment #8565255 - Flags: approval-mozilla-b2g37?
Attachment #8565255 - Flags: approval-mozilla-b2g37+
Attachment #8565255 - Flags: approval-mozilla-b2g34?
Attachment #8565255 - Flags: approval-mozilla-b2g34+
(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.
Flags: needinfo?(chens)
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
Attachment #8578567 - Flags: review?(cpearce)
Attachment #8578567 - Flags: review?(cpearce) → review+
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
Attachment #8578567 - Flags: approval-mozilla-b2g34?
Comment on attachment 8578567 [details] [diff] [review]
[2.1]_AdjustCurrentTimeForSlide_v4_rebase0317.patch

Let's go with the last approval.
Attachment #8578567 - Flags: approval-mozilla-b2g34?
See Also: → 1148049
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Blocks: 1243608
(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.
Flags: needinfo?(kikuo)
Depends on: 1284700
You need to log in before you can comment on or make changes to this bug.