1151775 - [Flame][Video]Share a video to mail/Messages and then back to Video, the video can't be played.

Status: VERIFIED FIXED

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

Description

[Elie]

[1.Description]:
[Flame][v2.2&3.0][Video]Try to share a video to mail/Messages and then back to Video by card video, tap play, "Cannot play video " pops up.
Attachment:VIDEO0342_Compress.MP4 and logcat_1627.txt
Happen time:16:27

[2.Testing Steps]: 
1.Launch Video
2.Tap a video to play
3.Tap menu ->Share->Tap mail
4.Long press Home to enter card view->switch to Video
5.Tap video to play

[3.Expected Result]: 
5.Video can be played normally.

[4.Actual Result]: 
5.It shown that "Cannot play video "

[5.Reproduction build]: 
Device:Flame 2.2 (Affected)
Build ID               20150406002503
Gaia Revision          a6351e1197d54f8624523c2db9ba1418f2aa046f
Gaia Date              2015-04-03 22:06:41
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c3335a5d3063
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150406.040047
Firmware Date          Mon Apr  6 04:00:58 EDT 2015
Bootloader             L1TC000118D0

Device:Flame 3.0 (Affected)
Build ID               20150406160205
Gaia Revision          834385f4c834238a4306bf87cc4be41615d91ff0
Gaia Date              2015-04-06 19:41:47
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/a530b5c3b713
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150406.194015
Firmware Date          Mon Apr  6 19:40:27 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

[Elie]

Attachment: logcat_1627.txt

Comment 2

[Elie]

Attachment: VIDEO0342_Compress.MP4

Comment 3

[Eric Chang [:ericcc] [:echang]]

Check for regression? V2.1? Thanks.

Comment 4

[Elie]

(In reply to Eric Chang [:ericcc] [:echang] from comment #3)
> Check for regression? V2.1? Thanks.

This issue exists on flame 2.1:
Flame 2.1:
Build ID               20150407001214
Gaia Revision          87e55a7ec688138812181747f690fd188d2a0668
Gaia Date              2015-04-03 21:43:01
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/9b0cebf723b9
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Reproduce rate         5/5

Comment 5

[Eric Chang [:ericcc] [:echang]]

Legacy issue, functionality not working, suggest to backlog, thanks.

Comment 6

[Hema Koka [:hema]]

Putting it in the priority queue. Also Ni Russ so it is in his radar.

Comment 7

[Russ Nicoletti [:russn]]

I didn't think there could be another use case for the "video cannot be played" error, but, this bug shows there is. 

This bug is similar to bug 1085212 in that they start with the user initiating a share activity while viewing a video and before completing the share activity the video app needs to view or play a video. In the case of bug 1085212, the result of trying to view a video as part of a pick activity while in the middle of a share activity resulted in contention for the video hardware. The solution was for the video app to release the video before initiating a share activity and restoring the video when the share activity completed.

However, this bug illustrates there is a corner case we didn't anticipate regarding the release/restore video logic: if the user moves the share activity to the background by long pressing the home button and returns to the video app, the video app doesn't know it was in the middle of a share activity so it hasn't yet restored the video. Consequently, the 'src' attribute of the video player is null; when the user tries to play the video, it doesn't play within the given amount of time resulting in the "cannot play video" error.

Something to note is this scenario only happens when sharing with the email app where the email app is opened to complete the share activity. When sharing with the SMS app, the task manager shows only the video app; the SMS app doesn't appear and therefore it's not possible to "return" to the video app in the middle of the share activity.

I can think of two ways to fix this. In both cases, there would be logic added to the video app to detect when it is coming to the foreground when a share activity is in progress.

1) Display a "the sharing activity must be completed before using the video app" overlay until the share activity is completed.
2) Restore the video.

I am leaning towards 1) because it doesn't seem proper to allow the user to play a video while that video is being shared.

NI for David to comment on the proposed solutions.

Comment 8

[David Flanagan [:djf]]

Yuck. Here are my thoughts:

The difference between SMS and email is that SMS does an inline activity and email does a window activity. In https://bugzilla.mozilla.org/show_bug.cgi?id=1085212#c7 John thought that we wouldn't have this problem for window activities. He said they would fire onsuccess right away, so we'd immediately restore the video. He was wrong, or this has changed since then, I guess.

If I understand correctly, one way to make this bug go away would be to revert bug 1085212. Obviously, though, we can't do that.

In the inline activity we did not get visibility change events, which was the source of the original bug. So we were forced to release the video when we launched the activity.

For a window activity, we should get the visibility change events, and we don't actually need to have the video released, so it should be okay to restore the video when if become visible again (or even when we are first hidden) after starting the activity.  Because both sides of the share activity are in completely separate apps in this case, they should be able to share the video hardware even if the user switches back and forth.

So I think it is okay to restore the video on visibility change. The trick will be to do it right so that we never restore the video twice or restore a video after the activity has ended.  Actually, I think that with window activities we may never get an onsuccess or onerror, so we really must restore the video on visibility change.

I'd really like to avoid having a new overlay with a new error message, because it is probably too late to localize it and it would be nice to fix this in 2.2.  And also because we should make things just work if we can.  For window activities there is no defined way to "complete the sharing activity", so we can't really wait for that to happen.

If this doesn't make sense, we can chat on IRC.  Let's try for a logic-only fix ASAP so we can nominate it into 2.2.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] russnicoletti:bug-1151775 > mozilla-b2g:master

Comment 10

[Russ Nicoletti [:russn]]

Comment on attachment 8596762 [details] [review]
[gaia] russnicoletti:bug-1151775 > mozilla-b2g:master

This PR restores the video when the app comes to the foreground if it has been released prior to a share activity.

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8596762 [details] [review]
[gaia] russnicoletti:bug-1151775 > mozilla-b2g:master

See my comments on github. I don't think it is right to use a global sharingActivity state variable.

Comment 12

[Russ Nicoletti [:russn]]

Comment on attachment 8596762 [details] [review]
[gaia] russnicoletti:bug-1151775 > mozilla-b2g:master

David, I've addressed your comments (I don't know why I thought adding a new variable was necessary; I think it's because for some reason I thought that 'videoHardwareReleased' was false when the app was coming back to the foreground in the STR, although why I thought that I don't know)

Regarding "disposition=window" activities potentially not completing, do you mean where the initiator will not ever get an onsuccess or onerror callback? I'm finding that killing the email app in the middle of the activity results in the 'onerror' callback being called. For my information, can you explain under what circumstances the activity will not complete?

Comment 13

[David Flanagan [:djf]]

Comment on attachment 8596762 [details] [review]
[gaia] russnicoletti:bug-1151775 > mozilla-b2g:master

This looks good to me, Russ. Assuming you think this is safe, please request uplift to 2.2.

About disposition=window activities possibly never completing, I just mean that the app that gets invoked may never call postResult() or postError() on the activity.  The user could share a video with the email app, then start composing an email, then go back to the video app without ever sending it.

In the earlier bug, John asserted (incorrectly, apparently) that for window activities we'd get an onsuccess immediately. That does seem like it would be the best thing. Then you'd know that your blob had been shared with some other app and you'd be done. As it stands there really is no way to know what the outcome of the activity is.

Comment 14

[Russ Nicoletti [:russn]]

Comment on attachment 8596762 [details] [review]
[gaia] russnicoletti:bug-1151775 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1085212 ("can't play video received from the video app via an inline share activity because the hardware is already in use")

[User impact] if declined: While a video is being shared with the email app, the user will not be able to use the video app to play any videos until the user either sends the email or explicitly cancels the email. The video app is essentially unusable until the user does one or the other. The biggest user impact would be if the user decides to wait or forgets about sending that email; in the meantime the video app would not be available to play videos.

[Testing completed]: unit tests and manual tests

[Risk to taking this patch] (and alternatives if risky): minimal risk, two line change to video app code. Alternative is to live with the bug.

[String changes made]: none

Comment 15

[Russ Nicoletti [:russn]]

Master: https://github.com/mozilla-b2g/gaia/commit/5d0d1049a520625c5ad214bdfacb850ef423b5f5

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 8596762 [details] [review]
[gaia] russnicoletti:bug-1151775 > mozilla-b2g:master

making an exception on approving this non blocking bug given the low risk. Lets verify this and if we have any fallouts we'd backout.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/a9f4cdc1476b11b07e032f222ba945c7a2cb0e97

Comment 18

[Norry.L.F(Leave from Mozilla)]

Hi zouyi,

Could you help verify this bug? thanks.

Comment 19

[zouyi(Leave from Mozilla)]

Attachment: verified_v2.2&3.0.mp4

According to the STR of Comment 0, this bug has been successfully verified on latest Flame v2.2&3.0,Nexus5 v2.2&3.0
Actual results:Video is played normally.
See attachment: verified_v2.2&3.0.mp4
Reproduce rate: 0/6
Device: Flame 2.2 build(Pass)
Build ID               20150505002501
Gaia Revision          772a9491909abd02dc67278dd453746e2dd358a8
Gaia Date              2015-05-05 02:02:24
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2df83538ae20
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150505.041743
Firmware Date          Tue May  5 04:18:00 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (Pass)
Build ID               20150505010204
Gaia Revision          70077825aab2c7a79611befb40a5fe7e610d5443
Gaia Date              2015-05-04 18:09:33
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/102d0e9aa9e1
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150505.043622
Firmware Date          Tue May  5 04:36:34 EDT 2015
Bootloader             L1TC000118D0


Device: Nexus5 2.2 (Pass)
Build ID               20150505002501

Gaia Revision          772a9491909abd02dc67278dd453746e2dd358a8

Gaia Date              2015-05-05 02:02:24

Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2df83538ae20

Gecko Version          37.0

Device Name            hammerhead

Firmware(Release)      5.1

Firmware(Incremental)  eng.cltbld.20150505.041235

Firmware Date          Tue May  5 04:12:53 EDT 2015

Bootloader             HHZ12f

Device: Nexus5 3.0 (Pass)
Build ID               20150505160203
Gaia Revision          42dc5f02a9df006b129824cd9bffa93cab937ab2
Gaia Date              2015-05-05 11:06:17
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/5907a8eca521
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150505.192812
Firmware Date          Tue May  5 19:28:29 EDT 2015
Bootloader             HHZ12f

Comment 20

[Norry.L.F(Leave from Mozilla)]

Hi bhavana,

Thanks for your patch.

And this bug is fixed on v2.2&v3.0, could you confirm whether it will be fixed and uplift to v2.1 or not?

Thanks again.

Comment 21

[Norry.L.F(Leave from Mozilla)]

Hi Mike,

According to commen 20, could you help with this bug? thanks :)

Comment 22

[Mike Lien[:mlien]]

Hi Norry,

Since this issue is not regression, it won't be uplift to v2.1. But we need to verify if it can be reproduced by v2.1S. If yes, please need info Steven Yang directly for judging whether v2.1S will adopt the fix or not, thanks.

Mark qawanted for v2.1S branch check

Comment 23

[zouyi(Leave from Mozilla)]

Attachment: verify_v2.1s.MP4

The bug can be reproduced on latest build of v2.1S_512mb&v2.1S_256mb.
See attachments:verify_v2.1s.MP4,logcat001.txt
Device: v2.1S_512mb build
Build ID               20150602001203
Gaia Revision          e29043077da834854bfda9d409d1e690fb8070c3
Gaia Date              2015-06-01 10:35:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/6c3b01e6da3c
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

Device: v2.1S_256mb build
Build ID               20150602001203
Gaia Revision          e29043077da834854bfda9d409d1e690fb8070c3
Gaia Date              2015-06-01 10:35:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/6c3b01e6da3c
Gecko Version          34.0
Device Name            scx15_sp7715ga
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150602.034746
Firmware Date          Tue Jun  2 03:47:59 EDT 2015

Comment 24

[zouyi(Leave from Mozilla)]

Attachment: logcat001.txt

Comment 25

[zouyi(Leave from Mozilla)]

Hi Steven,
Could you confirm whether it will be fixed and uplift to v2.1S or not?

Comment 26

[Steven Yang [:styang]]

(In reply to zouyi from comment #25)
> Hi Steven,
> Could you confirm whether it will be fixed and uplift to v2.1S or not?

Given there's clear error messages for users, won't fix for 2.1S at this stage. thanks.

Comment 27

[Norry.L.F(Leave from Mozilla)]

According to comment 26, close this bug.