1131951 - [dolphin & flame][video][FFOS7715 v2.1] Rapidly draging the scroll bar, cause video controls breaks

Status: VERIFIED FIXED

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

Description

[[e-mail]]

DEFECT DESCRIPTION:
  Rapidly draging the scroll bar, cause video controls breaks

Steps to reproduce:
----------------------------------------------------
Step 1: Open Video app, 
Step 2: Playing a video, 
Step 3: Rapidly draging the scroll bar, cause video controls breaks, video cannot playing until manually killing the Video app and open it again.

This issue can repro on both dolphine & flame.

Comment 1

[[e-mail]]

Dear Vchen & Shawn:

I have captured a video with flame & dolphin to clarify this issue more detail, please go and download at the blow address:
http://pan.baidu.com/s/1dDw8QnZ

Also the video resource in this test:
http://pan.baidu.com/s/1eQ3s0Sy

When I rapidly draging the scroll bar, cause video controls breaks, play/pause, fast foreword, and rewind are broken.

Because this issue also occurs in flame, please help to investigate this issue.

Thanks a lot!

Comment 2

[[e-mail]]

Dear Vchen & Shawn:

After some debug about this issue, I found when video doing seeking operation, seeking caused its readyState attribute to change to a value lower than HAVE_FUTURE_DATA, then a waiting event will be fired at the element, if seeking done, the element will fire timeupdate event and could playing at the new position.

But when I draging the scroll bar to seek to the time which aTime=0, then draging to another position, this operation cause the video broken. readyState attribute to change to a value lower than HAVE_FUTURE_DATA, then a waiting event fired at the element, but it's Always waiting and do not fire the timeupdate event, thus gaia do not receive the timeupdate event, and then pop-up the "another app is using video player" dialog.

I don't know what thing blocks the seeking operation, and cause the video cannot playing again. 

Please help to investigate this issue.

Thanks very much!

Comment 3

[Sotaro Ikeda [:sotaro]]

Bug 1112519 is also related to rapid seeking.

Comment 4

[[e-mail]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Bug 1112519 is also related to rapid seeking.
Dear Sotaro:
After applying the patch to 2.1, when drag progess bar to end and then back to beginning, the video can playing continue, but in onother case, playing video at the begin time, when Keep clicking the rewind button with some times, video also got broken and cannot playing again.

so this issue still exits.

Thanks!

Comment 5

[Sotaro Ikeda [:sotaro]]

lin,m did you correctly apply the patch? The patch could not be applied cleanly. I created attachment 8565494 [details] [diff] [review] in Bug 1112519. It is a patch for b2g v2.1. Can you test again?

On my side, the seeking problem was fixed by attachment 8565494 [details] [diff] [review].

Comment 6

[[e-mail]]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> lin,m did you correctly apply the patch? The patch could not be applied
> cleanly. I created attachment 8565494 [details] [diff] [review] in Bug
> 1112519. It is a patch for b2g v2.1. Can you test again?
> 
> On my side, the seeking problem was fixed by attachment 8565494 [details] [diff] [review]
> [diff] [review].
I'm so sorry to delay so many days, and thanks for your help.

I applied the patch correctly just now, the patch for b2g v2.1 in attachment 8565494 [details] [diff] [review] has changes are this:
> +    mDropAudioUntilNextDiscontinuity = false;
> +    mDropVideoUntilNextDiscontinuity = false;

This changes have already build and flash to the device successfully, but issue still exits, when I draging the scroll bar to the start of the video but not go to the end time, the video got broken and cannot playing again until I reloaded Video app.

Comment 7

[[e-mail]]

Dear Sotaro-

As Comment 6 says, after applying the patch you provided（ps:that modification has already landed our 2.1s branch）, the issue still exits, I got two ways to repro this issue, and I also attached one video to clarify the issue more details.

Video adress as follows:
http://pan.baidu.com/s/1pJ80zMj

In my operation,  issue occurs when draging the slider or rapidly tap the rewind button go to the start of the video, please help to check this bug more dig and find a better way to fix it.

Thanks so much!

Comment 8

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

upload the reproduced video to youtube:

https://www.youtube.com/watch?v=T_IWvQS-Bo4&feature=youtu.be

Also ni Beatriz to comment on the severity of this issue

Comment 9

[Sotaro Ikeda [:sotaro]]

I checked on v2.1 flame-kk, the problem did not happen. Therefore, this problem seems to be related to only dolphin now.

Comment 10

[Sotaro Ikeda [:sotaro]]

I had a chance to debug on v2.1s dolphon. But now the dolphin was bricked. Therefore, I could not debug on dolphin anymore :-(

Before the dolphin was bricked, I confirmed the following.
- During the seeking, there was a case that MediaDecoderStateMachine owns 3 video buffer and one video buffer is on compositor. And MediaDecoderStateMachine requested one more video frame. When this happens with the video, OMXCodec failed to decode video because of out-of-video-buffer on OMXCodec side.

Therefore, OMXCodec on dolphin seems to provide less video frames to owner than flame-kk's one.

MediaDecoderStateMachine is basically hold at most 3 video buffers, one more video buffer request is added by incorrect EnsureVideoDecodeTaskQueued() call in MediaDecoderStateMachine::CheckIfSeekComplete(). CheckIfSeekComplete() was called before DecodeSeek() call.

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/file/fc6db70a7ea2/content/media/MediaDecoderStateMachine.cpp#l1000

MediaDecoderStateMachine::IsVideoSeekComplete() checks mCurrentSeekTarget and mDropVideoUntilNextDiscontinuity. On b2g v2.1/v2.1s, these are set in MediaDecoderStateMachine::EnqueueDecodeSeekTask(). It causes CheckIfSeekComplete() was called before DecodeSeek() call.

mCurrentSeekTarget and mDropVideoUntilNextDiscontinuity should be set in MediaDecoderStateMachine::DecodeSeek(). This fix was done by Bug 1121692.

Comment 11

[[e-mail]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> I had a chance to debug on v2.1s dolphon. But now the dolphin was bricked.
> Therefore, I could not debug on dolphin anymore :-(
> 
> Before the dolphin was bricked, I confirmed the following.
> - During the seeking, there was a case that MediaDecoderStateMachine owns 3
> video buffer and one video buffer is on compositor. And
> MediaDecoderStateMachine requested one more video frame. When this happens
> with the video, OMXCodec failed to decode video because of
> out-of-video-buffer on OMXCodec side.
> 
> Therefore, OMXCodec on dolphin seems to provide less video frames to owner
> than flame-kk's one.
> 
> MediaDecoderStateMachine is basically hold at most 3 video buffers, one more
> video buffer request is added by incorrect EnsureVideoDecodeTaskQueued()
> call in MediaDecoderStateMachine::CheckIfSeekComplete().
> CheckIfSeekComplete() was called before DecodeSeek() call.
> 
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/file/fc6db70a7ea2/
> content/media/MediaDecoderStateMachine.cpp#l1000
> 
> MediaDecoderStateMachine::IsVideoSeekComplete() checks mCurrentSeekTarget
> and mDropVideoUntilNextDiscontinuity. On b2g v2.1/v2.1s, these are set in
> MediaDecoderStateMachine::EnqueueDecodeSeekTask(). It causes
> CheckIfSeekComplete() was called before DecodeSeek() call.
> 
> mCurrentSeekTarget and mDropVideoUntilNextDiscontinuity should be set in
> MediaDecoderStateMachine::DecodeSeek(). This fix was done by Bug 1121692.
Thanks for your clarification, learned it.

Now I get a workaround in gaia side, please help to review this modification.
>> function seekVideo(seekTime) {
>>     if (seekTime >= player.duration || seekTime < 0) {
>>       if (isLongPressing) {
>>         stopFastSeeking();
>>       }
>>       if (seekTime >= player.duration) {
>>         seekTime = player.duration;
>>         pause();
>>       }
>>       else {
>>         seekTime = 0;
>>       }
>>     }
>>     player.fastSeek(seekTime);
>>  }
Our workaround is change the seekTime to 0.000001, furthermore, we also must change this:
>> function handleSliderTouchMove(event) {
>>   if (!dragging) {
>>     return;
>>   }
>>   var touch = event.changedTouches.identifiedTouch(touchStartID);
>>   // We don't care the event not related to touchStartID
>>   if (!touch) {
>>     return;
>>   }
>>   var pos = (touch.clientX - sliderRect.left) / sliderRect.width;
>>   pos = Math.max(pos, 0);
>>   pos = Math.min(pos, 1);
>>   // Update the slider to match the position of the user's finger.
>>   // Note, however, that we don't update the displayed time until
>>   // we actually get a 'seeked' event.
>>   var percent = pos * 100 + '%';
>>   dom.playHead.classList.add('active');
>>   dom.playHead.style.left = percent;
>>   dom.elapsedTime.style.width = percent;
>>   dom.player.fastSeek(dom.player.duration * pos);
>> }
dom.player.fastSeek(dom.player.duration * pos)  ====> dom.player.fastSeek(dom.player.duration * pos + 0.000001);
Those modification can solve this bug easily, do you have any suggestion about this change?

Thank you.

Comment 12

[[e-mail]]

Attachment: SPRD_402598_workaround.patch

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> I had a chance to debug on v2.1s dolphon. But now the dolphin was bricked.
> Therefore, I could not debug on dolphin anymore :-(

My dolphin un-bricked via fastboot rom flash :-)

Comment 14

[Sotaro Ikeda [:sotaro]]

> dom.player.fastSeek(dom.player.duration * pos)  ====>
> dom.player.fastSeek(dom.player.duration * pos + 0.000001);
> Those modification can solve this bug easily, do you have any suggestion
> about this change?

Hmm, I do not think gaia side workaround is good idea. Any web content could hit this problem.

Comment 15

[Sotaro Ikeda [:sotaro]]

Bug 1121692 is very large patch to uplift to v2.1s. In Bug 1121692, we need only the following.
- Part 5 - Move the interesting seek state logic into DecodeSeek.

I locally applied it to v2.1s, the problem seems to be fixed.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch for v2.1s - Move the interesting seek state logic into DecodeSeek

attachment 8549972 [details] [diff] [review] from Bug 1121692
- Part 5 - Move the interesting seek state logic into DecodeSeek.

Comment 17

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8575354 [details] [diff] [review]
patch for v2.1s - Move the interesting seek state logic into DecodeSeek

lin.hui, can you check if the patch fix the problem?

Comment 18

[[e-mail]]

Comment on attachment 8575354 [details] [diff] [review]
patch for v2.1s - Move the interesting seek state logic into DecodeSeek

Dear sotaro：

Thanks for your reply, and after applying your new patch, this issue fixed, so could you help to check-in this patch on mozilla 2.1s branch?

Thank you.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to [e-mail] from comment #18)
> Comment on attachment 8575354 [details] [diff] [review]
> patch for v2.1s - Move the interesting seek state logic into DecodeSeek
> 
> Dear sotaro：
> 
> Thanks for your reply, and after applying your new patch, this issue fixed,
> so could you help to check-in this patch on mozilla 2.1s branch?
> 
> Thank you.

I am glad that the patch fixes the problem :-)  I just requested "2.1S?".

Comment 20

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

Comment on attachment 8575354 [details] [diff] [review]
patch for v2.1s - Move the interesting seek state logic into DecodeSeek

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

Assuming this passes mochitests, r+.

Comment 21

[Beatriz Rodríguez [:brg]]

Sorry for getting late here. 
I have been testing today with yesterday partner build(not sure if this patch is already included) but I cannot reproduce the issue.

I am happy to see that there is already a patch in this bug and according to comment 18, it seems to fix the issue. IMHO we should land it in 2.1S.

Comment 22

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb72bc2b3fd3

Comment 23

[[e-mail]]

(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb72bc2b3fd3
Dear Sotaro -

As customer says, this fix should land in 2.1s, so does this fix has already landed in mozilla 2.1s?

Thanks.

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to [e-mail] from comment #23)
> (In reply to Sotaro Ikeda [:sotaro] from comment #22)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb72bc2b3fd3
> Dear Sotaro -
> 
> As customer says, this fix should land in 2.1s, so does this fix has already
> landed in mozilla 2.1s?
> 
> Thanks.

I set checkin-needed flag, but it was cleared by RyanVM. I am not sure the reason of it.

Comment 25

[Sotaro Ikeda [:sotaro]]

RyanVM, can you commit the patch to b2g-v2.1s?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

The devices team manages v2.1s, that's why.

Comment 27

[Vincent Liu[:vliu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> RyanVM, can you commit the patch to b2g-v2.1s?

Hi Sotaro,

Could you please rearrange your patch to HG format? Thanks.

Comment 28

[Sotaro Ikeda [:sotaro]]

(In reply to Vincent Liu[:vliu] from comment #27)
> (In reply to Sotaro Ikeda [:sotaro] from comment #25)
> > RyanVM, can you commit the patch to b2g-v2.1s?
> 
> Hi Sotaro,
> 
> Could you please rearrange your patch to HG format? Thanks.

It is OK, but why you requested it? I used the following command. It should be compatible to "hg diff -p -U 8" in [1]. And I never received the such request until now. I just want to know the reason.

> git diff -p -U8

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F

Comment 29

[Vincent Liu[:vliu]]

I am sorry about not clearly explaining my concern. When I saw your patch, it didn't have author information in it. As I know the detailed information would be: 

# HG changeset patch
# User <Author>
# Date 
#      Mon Mar 16 15:07:22 2015 +0800
# Node ID XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
# Parent  XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX

I believe your patch is compatible to "hg diff -p -U 8". I only want to focus on the most complete information in branch code. With this information, it is better for further tracking. Please correct me if I got anything wrong. Thanks

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch for v2.1s - Move the interesting seek state logic into DecodeSeek

Carry "r=cpearce".

Comment 31

[Sotaro Ikeda [:sotaro]]

vliu, how about attachment 8580053 [details] [diff] [review].

Comment 32

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/a9095b15412f

Hi Sotaro,

Now I know |hg qimport -e -P  xxx.patch| will generate Author info if adding |From: your name <e-mail>| in your patch. It is simple to me to add it next time I got the same situation.

Comment 33

[Sotaro Ikeda [:sotaro]]

(In reply to Vincent Liu[:vliu] from comment #32)
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/a9095b15412f
> 
> Hi Sotaro,
> 
> Now I know |hg qimport -e -P  xxx.patch| will generate Author info if adding
> |From: your name <e-mail>| in your patch. It is simple to me to add it next
> time I got the same situation.

Thanks!

Comment 34

[Boon]

This bug has been verified as "pass" on latest v2.1s (512mb&256mb) using the test video of comment 1 by the STR in Comment 0.

Actual results: Video controls will not break anymore and video can always play normally.
See attachment: verified_v2.1s.3gp
Reproduce rate: 0/15

Device: v2.1s_256mb (Verified)
Build ID               20150623001206
Gaia Revision          4196ca9119b4cde8353130165de90c0ffa6060a3
Gaia Date              2015-06-17 03:30:30
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/6299ba592af4
Gecko Version          34.0
Device Name            scx15_sp7715ga
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150623.035742
Firmware Date          Tue Jun 23 03:57:55 EDT 2015

Device: v2.1s_512mb (Verified)
Build ID               20150623001206
Gaia Revision          4196ca9119b4cde8353130165de90c0ffa6060a3
Gaia Date              2015-06-17 03:30:30
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/6299ba592af4
Gecko Version          34.0
Device Name            scx15_sp7715ea
Firmware(Release)      4.4.2
Firmware(Incremental)  122
Firmware Date          Thu Feb  5 12:42:58 CST 2015

Comment 35

[Boon]

Attachment: verified_v2.1s.3gp