1147090 - Flame: Coming back from lockscreen, video player does not show frame at 00:00 frame

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[No-Jun Park [:njpark]]

STR:
Open video player, select video, pause, then rewind to 00:00
Press power button to go to lockscreen
Press power again and resume video app

Expected:
Video screen is shown
Actual:
Video screen is blank.

This does not reproduce in 3.0, and only reproduces in 2.2

Version Info:
Gaia-Rev        014d38f7ad3912b8b33cb08ce7535a5dc5aced59
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7a9f2a248e57
Build-ID        20150324002504
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150324.041652
FW-Date         Tue Mar 24 04:17:03 EDT 2015
Bootloader      L1TC000118D0

Sotaro thinks Bug 1062134 might be causing this regression

Comment 1

[No-Jun Park [:njpark]]

Reason for blocking nomination:
Regression, and bad UX behavior that can be encountered in common use scenarios.

Comment 2

[Sotaro Ikeda [:sotaro]]

I am going to investigate.

Comment 3

[Sotaro Ikeda [:sotaro]]

On master, the problem does not happen. It seems to be fixed by Bug 1135170. I do not think that it could be uplifted to b2g-v2.2. The change and dependent bugs are a lot.

Comment 4

[Sotaro Ikeda [:sotaro]]

MediaDecoderStateMachine does not load first video frame when it is resumed from dormant state since Bug 1062134 fix. MediaDecoderStateMachine expects video frame update by playback or seek.

In this bug's STR, seek should trigger video frame update. But until Bug 1135170 fix, seeks does not update video frame by seek if seeking time is same. The related code is in MediaDecoderStateMachine::DecodeSeek().

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/cb4856ecd781/dom/media/MediaDecoderStateMachine.cpp#l2463

Comment 5

[Sotaro Ikeda [:sotaro]]

From comment 4, we could say that this bug is a regression of Bug 1062134.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2g v2.2 - Change seek skip check

The patch fixed the problem on v2.2 flame.

Comment 7

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

Comment on attachment 8582700 [details] [diff] [review]
patch for b2g v2.2 - Change seek skip check

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2485,5 @@
>  
>    mDecodeToSeekTarget = false;
>  
> +  if (!currentTimeChanged &&
> +      !(HasVideo() && (VideoQueue().GetSize() == 0) && !VideoQueue().IsFinished())) {

Does it make sense for this to be:

if (!currentTimeChanged &&
    (!HasVideo() || (VideoQueue().GetSize() == 0 && !VideoQueue().IsFinished())))

Then we still take the !currentTimeChanged when seeking in an audio-only file.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #7)
> Comment on attachment 8582700 [details] [diff] [review]
> patch for b2g v2.2 - Change seek skip check
> 
> Review of attachment 8582700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +2485,5 @@
> >  
> >    mDecodeToSeekTarget = false;
> >  
> > +  if (!currentTimeChanged &&
> > +      !(HasVideo() && (VideoQueue().GetSize() == 0) && !VideoQueue().IsFinished())) {
> 
> Does it make sense for this to be:
> 
> if (!currentTimeChanged &&
>     (!HasVideo() || (VideoQueue().GetSize() == 0 &&
> !VideoQueue().IsFinished())))
> 
> Then we still take the !currentTimeChanged when seeking in an audio-only
> file.

I assume you mean the following. It is a logical equivalence to attachment 8582700 [details] [diff] [review]. Yeah, it makes the intent more clear :-)

if (!currentTimeChanged &&
    (!HasVideo() || !(VideoQueue().GetSize() == 0 && !VideoQueue().IsFinished())))

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2g v2.2 - Change seek skip check

Apply the comment. Carry "r=cpearce".

Comment 10

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8583069 [details] [diff] [review]
patch for b2g v2.2 - Change seek skip check

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 1062134
User impact if declined: User might not see the video frame when the video app goes back to foreground.
Testing completed: locally tested.
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 11

[No-Jun Park [:njpark]]

The patch is tested on 2.2, and it looks good.

Tested Gecko commit: efdb447485a5d67b783ba890520787a1c0e6638f

Comment 12

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on latest Nightly build of Flame v2.2 and Nexus 5 v2.2 by the STR in Comment 0.

Actual results: Coming back from lockscreen, video screen/frame is shown normally at 00:00 frame.
See attachment: verified_Flame_v2.2.3gp
Reproduce rate: 0/10


Device: Flame v2.2 (Verified) 
Build ID               20150706002507
Gaia Revision          ea11f422b687a982f0a961c9aea7858066561707
Gaia Date              2015-07-02 23:37:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0214b4c1ea0
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150706.035706
Firmware Date          Mon Jul  6 03:57:18 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus5 v2.2 (Verified) 
Build ID               20150706002507
Gaia Revision          ea11f422b687a982f0a961c9aea7858066561707
Gaia Date              2015-07-02 23:37:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0214b4c1ea0
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150706.041056
Firmware Date          Mon Jul  6 04:11:15 EDT 2015
Bootloader             HHZ12f

Comment 15

[Shally Li（leave from Mozilla）]

Attachment: verified_Flame_v2.2.3gp