1146304 - [Video][dolphin][FFOS7715 v2.1s] Touch slider bar or tap forward button, the video got stuck 1s then resume playing.

Status: VERIFIED FIXED

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

Description

[[e-mail]]

DEFECT DESCRIPTION:
  Touch slider bar or tap forward button, the video got stuck 1s then resume playing.

Steps to reproduce:
----------------------------------------------------
1.Playing video in Video app.
2.tap forward button to forward.
3.Video got stuck about 1 second, then resume playing.

Reproduce rate:  5/5

Comment 1

[[e-mail]]

Dear Vance -

According to mozilla bug 1112438, Slider doesn't go back to zero after the video play end. so we got the code change as follows:

diff --git a/content/media/MediaDecoderStateMachine.cpp b/content/media/MediaDecoderStateMachine.cpp
--- a/content/media/MediaDecoderStateMachine.cpp
+++ b/content/media/MediaDecoderStateMachine.cpp
@@ -2247,17 +2247,26 @@ MediaDecoderStateMachine::SeekCompleted(
   int64_t newCurrentTime = mCurrentSeekTarget.mTime;
 
   // Setup timestamp state.
   VideoData* video = VideoQueue().PeekFront();
   if (seekTime == mEndTime) {
     newCurrentTime = mAudioStartTime = seekTime;
   } else if (HasAudio()) {
     AudioData* audio = AudioQueue().PeekFront();
-    newCurrentTime = mAudioStartTime = audio ? audio->mTime : seekTime;
+    // Though we adjust the newCurrentTime in audio-based, and supplemented
+    // by video. For better UX, should NOT bind the slide position to
+    // the first audio data timestamp directly.
+    // While seeking to a position where there's only either audio or video, or
+    // seeking to a position lies before audio or video, we need to check if
+    // seekTime is bounded in suitable duration. See Bug 1112438.
+    int64_t videoStart = video ? video->mTime : seekTime;
+    int64_t audioStart = audio ? audio->mTime : seekTime;
+    newCurrentTime = mAudioStartTime =
+        std::min(std::min(audioStart, videoStart), seekTime);
   } else {
     newCurrentTime = video ? video->mTime : seekTime;
   }
   mPlayDuration = newCurrentTime - mStartTime;

This modification landed in 2.1s branch.

When this patch exit, playing video in Video app, Tap forward button or touch slider bar to the new play position which playing time is bigger than current, video will be stuck 1 second then resume playing, this may take very bad UX. remove that modification, this stuck issue gone. 

so please help find someone to check this issue.

Thanks so much!

Comment 2

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Kilik, according to Comment#1, this bug has something to do with the patch of Bug#1112438, could you help to check if you can refine the patch of Bug#1112438 to avoid this problem?

Thanks for your help

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

Sure, I'll look into it and let you know. 
I leave the ni? me as a reminder

Comment 4

[[e-mail]]

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #3)
> Sure, I'll look into it and let you know. 
> I leave the ni? me as a reminder
Hi kilik -

How about this issue? if any information required, Please let me know.

Thank you.

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

(In reply to [e-mail] from comment #4)
> (In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #3)
> > Sure, I'll look into it and let you know. 
> > I leave the ni? me as a reminder
> Hi kilik -
> 
> How about this issue? if any information required, Please let me know.
> 
> Thank you.

Sorry for late response, I was preparing the environment & testing if this phenomena also appears on flame and woodduck. And yes, sadly.
The phenomena appears because that patch always choose the minimal value among seekTime, audioStart, videoStart as the final newCurrentTime and mAudioStartTime, and the seekTime is alwasy smaller than audioStart/videStart, so that we will need to wait for a little time to see the A/V continue playing.

I suppose the logic should be refined

from

int64_t videoStart = video ? video->mTime : seekTime;
int64_t audioStart = audio ? audio->mTime : seekTime;
newCurrentTime = mAudioStartTime = std::min(std::min(audioStart, videoStart), seekTime);

to

int64_t videoStart = video ? video->mTime : seekTime;
int64_t audioStart = audio ? audio->mTime : seekTime;
int64_t minAVStart = std::min(audioStart, videoStart);
newCurrentTime = mAudioStartTime = minAVStart == seekTime ? seekTime : minAVStart;

This could make the A/V playing as soon as possible after seeking and also fix the misplaced slide in woodduck.

Hello Chris,
Would you like to feedback about the modification aforementioned.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

That modification sounds fine to me.

Comment 7

[[e-mail]]

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #5)
> I suppose the logic should be refined
> 
> from
> 
> int64_t videoStart = video ? video->mTime : seekTime;
> int64_t audioStart = audio ? audio->mTime : seekTime;
> newCurrentTime = mAudioStartTime = std::min(std::min(audioStart,
> videoStart), seekTime);
> 
> to
> 
> int64_t videoStart = video ? video->mTime : seekTime;
> int64_t audioStart = audio ? audio->mTime : seekTime;
> int64_t minAVStart = std::min(audioStart, videoStart);
> newCurrentTime = mAudioStartTime = minAVStart == seekTime ? seekTime :
> minAVStart;
Hi kilik -

After review, change the seekTime logic, this modification looks good to me. so could you help to check-in this modification on 2.1s？

Thank you for your help.

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_0] Choose audio or video start time as seektime if available.

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_1] Choose audio or video start time as seektime if available.

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_2] Choose audio or video start time as seektime if available.

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [master] Choose audio or video start time as seektime if available.

Comment 12

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Kilik, thanks for your help on fixing this one. ni Steven and Vincent to help to land this one on 2.1s

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8585844 [details] [diff] [review]
[2_0] Choose audio or video start time as seektime if available.

Review of attachment 8585844 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Chris,

This is a side-effect from applying patch in Bug 1112438
I did a logic refinement to make playback more responsive while seeking.
I suppose this modification will be applied to 2_0/2_1/2_2/master, so I prepared 4 patches against different branches.
Need your help to review them, thanks.

Comment 14

[Steven Yang [:styang]]

Give it a 2.1S+ for landing once got a review plus.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8585844 [details] [diff] [review]
[2_0] Choose audio or video start time as seektime if available.

Review of attachment 8585844 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaDecoderStateMachine.cpp
@@ +2023,2 @@
>          newCurrentTime = mAudioStartTime =
> +          minAVStart == seekTime? seekTime : minAVStart;

if (minAVStart == seekTime) then this logic is equivalent to:

newCurrentTime = mAudioStartTime = minAVStart;

if (minAVStart != seekTime) then this logic is equivalent to:

newCurrentTime = mAudioStartTime = minAVStart;

So why don't you just write

newCurrentTime = mAudioStartTime = minAVStart;

Or even:

newCurrentTime = mAudioStartTime = std::min(audioStart, videoStart);

Same comment for the other patches...

I hope that our "drop frames up to the seek target" will trim off samples we don't want to playback.

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Chris Pearce (:cpearce) from comment #15)
> Comment on attachment 8585844 [details] [diff] [review]
> [2_0] Choose audio or video start time as seektime if available.
> 
> Review of attachment 8585844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +2023,2 @@
> >          newCurrentTime = mAudioStartTime =
> > +          minAVStart == seekTime? seekTime : minAVStart;
> 
> if (minAVStart == seekTime) then this logic is equivalent to:
> 
> newCurrentTime = mAudioStartTime = minAVStart;
> 
> if (minAVStart != seekTime) then this logic is equivalent to:
> 
> newCurrentTime = mAudioStartTime = minAVStart;
> 
> So why don't you just write
> 
> newCurrentTime = mAudioStartTime = minAVStart;
> 
> Or even:
> 
> newCurrentTime = mAudioStartTime = std::min(audioStart, videoStart);
> 
> Same comment for the other patches...
> 
> I hope that our "drop frames up to the seek target" will trim off samples we
> don't want to playback.

Ah...what a silly mistake I made :(, thanks for reminding.

I did seek several times and verified the timestamp then checked DropVideoUpToSeekTarget() / DropAudioUpToSeekTarget() and i think by logic in [1][2], those samples we don't want to playback should be trimmed off.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#3055
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#3093

Comment 17

[[e-mail]]

(In reply to Chris Pearce (:cpearce) from comment #15)
> if (minAVStart == seekTime) then this logic is equivalent to:
> 
> newCurrentTime = mAudioStartTime = minAVStart;
> 
> if (minAVStart != seekTime) then this logic is equivalent to:
> 
> newCurrentTime = mAudioStartTime = minAVStart;
> 
> So why don't you just write
> 
> newCurrentTime = mAudioStartTime = minAVStart;
So the newCurrentTime always the min time of audio and video time in decodedqueue, I'm so sorry for not aware of this too.

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_0] Choose audio or video start time as seektime if available. (v2)

Comment 19

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_1] Choose audio or video start time as seektime if available. (v2)

Comment 20

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_2] Choose audio or video start time as seektime if available. (v2)

Comment 21

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [master] Choose audio or video start time as seektime if available. (v2)

Modified patches are attached - Code logic simplified.

Comment 22

[Kilik Kuo [:kikuo] (inactive)]

Try results -
[2_0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f84c6a473af8
[2_1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa396b086c1f
[2_2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=15ee0fc96f54
[master] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0acc4a647e2

It seems that the results are not green even there's no modification.

[2_0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d36cacba7113
[2_1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfe5d1bd053
[2_2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3526d4aea13d

Comment 23

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_0m] Choose audio or video start time as seektime if available. (v2)

Add r=cpearce in commit message, carry r+ from cpearce.

Comment 24

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_1] Choose audio or video start time as seektime if available. (v2)

Add r=cpearce in commit message, carry r+ from cpearce.

Comment 25

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [2_2] Choose audio or video start time as seektime if available. (v2)

Add r=cpearce in commit message, carry r+ from cpearce.

Comment 26

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [master] Choose audio or video start time as seektime if available. (v2)

Add r=cpearce in commit message, carry r+ from cpearce.

Comment 27

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8587221 [details] [diff] [review]
[2_0m] Choose audio or video start time as seektime if available. (v2)

Try result - Comment 22

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Side-effect after Bug 1112438 landed, choosing minimal value among AudioStartTime/VideoStartTime/SeekTime as target seek position. 
User impact if declined: User may take a little more time to see playback continuing after seek.
Testing completed: 2.0m/2.1s/2.2/master
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch:None

Comment 28

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8587223 [details] [diff] [review]
[2_2] Choose audio or video start time as seektime if available. (v2)

Try result - Comment 22

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Side-effect after Bug 1112438 landed, choosing minimal value among AudioStartTime/VideoStartTime/SeekTime as target seek position. 
User impact if declined: User may take a little more time to see playback continuing after seek.
Testing completed: 2.0m/2.1s/2.2/master
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch:None

Comment 29

[bhavana bajaj [:bajaj]]

Please seek approvla only after the code has landed on m-c.

Comment 30

[Kilik Kuo [:kikuo] (inactive)]

(In reply to bhavana bajaj [:bajaj] from comment #29)
> Please seek approvla only after the code has landed on m-c.

Got it, Thanks bajaj : )

Comment 31

[Kilik Kuo [:kikuo] (inactive)]

Patches for 2.1(this bug) & master are provided,
Try result ==> Comment 22

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb03bbdbd79

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bb03bbdbd79

Comment 34

[bhavana bajaj [:bajaj]]

Comment on attachment 8587223 [details] [diff] [review]
[2_2] Choose audio or video start time as seektime if available. (v2)

Approving as this is a fallout from 1112438 which is on 2.2

Comment 35

[bhavana bajaj [:bajaj]]

Comment on attachment 8587221 [details] [diff] [review]
[2_0m] Choose audio or video start time as seektime if available. (v2)

Don't think we'd take this in 2.0 at this point, given https://bugzilla.mozilla.org/show_bug.cgi?id=1112438 never landed there anyways but this branch is open for critical bugs only at this point.

does this patch apply to b2g34?

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d40ebc8e0642

(In reply to bhavana bajaj [:bajaj] from comment #35)
> does this patch apply to b2g34?

There's a v2.1 patch attached as well.

Comment 37

[bhavana bajaj [:bajaj]]

Comment on attachment 8587222 [details] [diff] [review]
[2_1] Choose audio or video start time as seektime if available. (v2)

Approving the 2.1 specific patch, hoping this applies cleanly

Comment 38

[[e-mail]]

Dear Kikuo -

We think this modification should land in 2.1s branch, when we do not change the seek logic, video always got 3-5 seconds stuck, this takes very bad UX, please help to land this patch in 2.1s.

Thanks to your great help.

Comment 39

[Kilik Kuo [:kikuo] (inactive)]

(In reply to bhavana bajaj [:bajaj] from comment #37)
> Comment on attachment 8587222 [details] [diff] [review]
> [2_1] Choose audio or video start time as seektime if available. (v2)
> 
> Approving the 2.1 specific patch, hoping this applies cleanly

Hello bajaj, in Comment 37, do you mean "approval-mozilla-b2g34+" but "approval-gaia-v2.1+" ?
Do I need to nominate it again ?

(In reply to bhavana bajaj [:bajaj] from comment #35)
> Comment on attachment 8587221 [details] [diff] [review]
> [2_0] Choose audio or video start time as seektime if available. (v2)
> 
> Don't think we'd take this in 2.0 at this point, given
> https://bugzilla.mozilla.org/show_bug.cgi?id=1112438 never landed there
> anyways but this branch is open for critical bugs only at this point.
> 
> does this patch apply to b2g34 ?

Sorry for misleading, I should use "[2_0m]" instead of "[2_0]" as the description for Attachment 8587221 [details] [diff]. 
2.0m is also affected by Bug 1112438.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/1ff8357b9b13

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/1ff8357b9b13

Comment 42

[[e-mail]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #41)
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/1ff8357b9b13

Thank you for your great support.

Comment 43

[bhavana bajaj [:bajaj]]

Looks like it landed everywhere its need to, clearing the NI.

Comment 44

[Cola, OOO till Oct. 8th]

Attachment: Flame v2.2.3gp

This bug has been verified as pass on latest build of Flame [v2.1&v2.2&Master] & Nexus5 [v2.2&Master] &v2.1s 512mb by the STR in Comment 0.

Actual results:When user drags the progress slider, it goes forward/backward smoothly.

See attachment:Flame v2.2.3gp

Reproduce rate: 0/10

Device: Flame 2.1 build(Pass)
Build ID               20150709161203
Gaia Revision          d13826b20b4a45e3f5cd4b25a30a737d8be7f1b9
Gaia Date              2015-07-02 23:36:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e6585848a94a
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150709.213937
Firmware Date          Thu Jul  9 21:39:49 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 2.2 build(Pass)
Build ID               20150709162504
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e002005dc994
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150709.202124
Firmware Date          Thu Jul  9 20:21:35 EDT 2015
Bootloader             L1TC000118D0

Device: Flame Master build(Pass)
Build ID               20150709160207
Gaia Revision          bdddfe1ebb796e2bc1c048d5c4e0f97f3d06f98b
Gaia Date              2015-07-09 11:58:52
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/adfdc7f29ba7
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150709.193215
Firmware Date          Thu Jul  9 19:32:27 EDT 2015
Bootloader             L1TC000118D0

Device: v2.1s 512mb build(Pass)
Build ID               20150708001206
Gaia Revision          c1a9fe0710a214df0dcabe89e45f3a9c41c282f9
Gaia Date              2015-07-03 02:24:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/9422877f1ea5
Gecko Version          34.0
Device Name            scx15_sp7715ea
Firmware(Release)      4.4.2
Firmware(Incremental)  145

Device: Nexus5 2.2 build(Pass)
Build ID               20150709162504
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e002005dc994
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150709.205934
Firmware Date          Thu Jul  9 20:59:53 EDT 2015
Bootloader             HHZ12f

Device: Nexus5 Master build(Pass)
Build ID               20150709160207
Gaia Revision          bdddfe1ebb796e2bc1c048d5c4e0f97f3d06f98b
Gaia Date              2015-07-09 11:58:52
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/adfdc7f29ba7
Gecko Version          42.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150709.191337
Firmware Date          Thu Jul  9 19:13:55 EDT 2015
Bootloader             HHZ12f

Comment 45

[SandKing]

Attachment: Video: Flame.3GP

This bug does not exsit on latest Flame v2.5&v2.6 and Aries KK v2.6.

STR: Same as comment 0

Actual results:When user drags the progress slider, it goes forward/backward smoothly.
See attachment: Flame.3gp
Reproduce rate: 0/10

Aries v2.6:
Build ID               20151107001102
Gaia Revision          c3436122d678911d04b8f491724596116890ff9b
Gaia Date              2015-11-06 18:22:03
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151106.232924
Firmware Date          Fri Nov  6 23:29:32 UTC 2015
Bootloader             s1

Device: Flame KK 2.5 512mb 
Build ID               20151108004501
Gaia Revision          577948202ae12154524a2bd2bd6d467838ad50b8
Gaia Date              2015-11-07 10:00:59
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ae7b8b1fd9e1504347b938820f99d75058049386
Gecko Version          44.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151108.045623
Firmware Date          Sun Nov  8 04:56:35 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame KK v2.6 512mb (master)
Build ID               20151108150206
Gaia Revision          c3436122d678911d04b8f491724596116890ff9b
Gaia Date              2015-11-06 18:22:03
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151108.183921
Firmware Date          Sun Nov  8 18:39:33 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0