869851 - [Video Player] Player size is not properly set when video app mimimized while playing and reopened in landscape mode

Status: VERIFIED FIXED

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

Description

[Leo]

1. Title : Video Player size is not properly set when video app mimimized while playing and reopened in landscape mode
2. Precondition : Playing video file in landscape mode
3. Tester's Action : (1) Open Video app-> Play a video
                     (2) Rotate to landscape mode -> press home button to minimize video app
                     (3) Reopen video app 
4. Detailed Symptom (ENG.) : Player size is not resized properly ( video component lies on left side of the screen).
5. Expected : video player should be properly resized.
6.Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: 3c48f7af4ecc8d32f62a44f0a30d4f18895bc33e
9.Personal email id: [e-mail]

Comment 1

[Leo]

Attachment: Pointer to pull request

While video is playing in landscape mode and if video player is minimized (pressing home button) and reopened, resize event occurs for two time:
1. resize to portrait mode (working fine since setPlayerSize is called)
2. resize to landscape mode (not working: setPlayerSize is called).

So added playerShowing condition when resize event occurs.

Please check the patch.

Comment 2

[Leo]

While video is playing in landscape mode and if video player is minimized (pressing home button) and reopened, resize event occurs for two times:
1. resize to portrait mode (working fine since setPlayerSize is called)
2. resize to landscape mode (not working: setPlayerSize is NOT called).

So added playerShowing condition when resize event occurs.

Please check the patch.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like a small fix with a high user facing win that we should get into v1.1

Comment 4

[David Flanagan [:djf]]

Comment on attachment 746878 [details]
Pointer to pull request

I can not reproduce the bug (on master), with a unagi device, so I can't really review the patch.  Or maybe I just don't know what I'm looking for... It it a subtle thing?  If you can still reproduce it, maybe attaching a screenshot would help.

Looking at the code, I don't know why we need the test for dom.player.readyState at all. I think the code would be best if that if statement was just if (playerShowing) {...}.

But I don't think we should land any changes unless there is a real bug here.

If this happens on some devices and not others, then it might be a clue to a gecko or gonk bug and we should investigate some more.

Also, your patch changes the permissions of the javascript file. We don't want files with executable bits set, so if you continue to work on this bug, please be sure to fix that.

Comment 5

[Leo]

Attachment: Video Attachment

Please see the attached video file. I am able to reproduce this issue in Unagi device also.
I have analyzed the issue completely. This is a small fix in Gaia code.
We need test "playerShowing" var in resize event every time to fix this issue. The Patch I have uploaded does the same.

I am also not aware why dom.player.readyState is tested in resize event.

David, Please let me know if submit patch with only testing "playerShowing" in resize event.

Comment 6

[Leo]

David, Please let me know if I submit patch with only testing "playerShowing" in resize event is enought for this issue.

Please confirm, I will upload new patch.

Comment 7

[David Flanagan [:djf]]

Thanks for the video.  I can see the bug, but I cannot reproduce it on master or v1-train.

In the video, the title bar shows the ".3gp" file extension. This means that you are testing on a phone that does not have bug 856542 applied. 

The patch for bug 856542 fixed a number of small video app bugs, and maybe it fixed this one as well. That bug has been uplifted to v1-train, so I think you ought to have it on your devices.  Could you just be using an out-of-date build?

Comment 8

[Leo]

Attachment: Video Attachment: with latest Master gaia

David, 
Thank you for the comments.
I have pushed latest Mozilla's master Gaia to Leo device and tested.
I can reproduce this issue but not 100% reproducible. Please see the attached video tested with latest mozilla's Gaia in Leo device.

Comment 9

[Leo]

Hi David,

Can you please provide your comments for this issue?
With the attached patch it is working fine. Please let me know your feedback.

Comment 10

[David Flanagan [:djf]]

I wrote a long comment about this bug last week. Maybe I forgot to click the "Save Changes" button.  (Or maybe I put the comment in the wrong bug?)  Thanks for pinging me about it.

I think the root cause of this bug is a race condition. When we go from the homescreen back to the already-running video app, two events are triggred: mozvisiblitychange and resize.  The visiblitychange event calls restoreVideo(), which calls setVideoURL().  But that function is asynchronous, and there is a race condition between the video player readyState changing and the resize event. If the resize occurs before the video is reloaded then setPlayerSize() does not get called.  If it occurs after, then the player is resized correctly.

setVideoUrl() takes a callback function that is run once the video is ready. setVideoUrl() is called twice in video.js.  Once, the callback function includes a call to setPlayerSize().  But the other time, when we call it from restoreVideo() we do not call setPlayerSize() in the callback.

I think that the best fix for this bug is to call setPlayerSize() in the callback function passed to setVideoUrl() in the restoreVideo() function. At video.js:912.

Would you try that and see if it works for you? If so, submit a pull request and ask for my review.  Please try not to change the file permissions in the pull request, however.

Comment 11

[Leo]

Attachment: Pointer to pull request

Hi David,

I have made changes as per your suggestions.
Calling setPlayerSize() in the callback function passed to setVideoUrl() in the restoreVideo() function.

Please review the patch

Comment 12

[David Flanagan [:djf]]

Comment on attachment 752533 [details]
Pointer to pull request

Looks good. I was never able to reproduce the bug, so I can't verify that it fixes the bug, but the patch is safe and should work.

r+ for commit 2b99478

Comment 13

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/5f1b3f37c152e34d2b651ca003e8a52c8f3e5dd8

This is a one-line patch that should uplift cleanly, so I'll leave the uplift for jhford.

Comment 14

[David Flanagan [:djf]]

This is a priority bug for the SD workweek. If jhford doesn't uplift soon, I'll do it myself.

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 5f1b3f37c152e34d2b651ca003e8a52c8f3e5dd8 to:
v1-train: 7af280e393fa9fb21e7bfc037abf57ab32a6e543

Comment 16

[:CSchmoeckel]

Added Video Suite Test Case #8550 [B2G Video] Verify playback resumes properly when the Video App is reopened in landscape video aspect

Comment 17

[Parul Mathur]

Executed test case in MozTrap https://moztrap.mozilla.org/manage/case/8550/
and verified fixed on:
Device: Leo phone
Build Identifier: 20130621070212
Update channel: leo/1.1/nightly
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a34f6d62cb05
Gaia: cca61224e6df8e9f7e450d77cb6a8cf91029be641371823447
Git commit info: 2013-06-21 14:04:07
OS version: 1.1.0.0-prerelease