1085212 - [MADAI][Multimedia] can't play video received from the video app via an inline share activity because the hardware is already in use

Status: VERIFIED FIXED

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

Description

[sangyeol]

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36 CoolNovo/2.0.9.20

Steps to reproduce:

1.start to play a small size video(under 1MB) - recording any with qcif size
2.enter the "menu" button.
3.choose the "Share" button.
4.choose the "Messages" button.
5.An video attachment success
6.choose the "Attachment" button in message app.(A clip shape icon)
7.choose the "Video" button.
8.choose a small size video(under 1MB) again


Actual results:

- Cannot attach a video file in the message app any more.
- Video list doesn't respond in the "Select a video view"

Some prior analysis>
when attach a video file, normally Shutdown() function in MediaDecoderStateMachine work properly.
But, in this case, Shutdown() function could not shutdown MediaDecoderStatemachine to load other decoder instance.


Expected results:

It is expected to attach multiple video files in the message app.

Comment 1

[sangyeol]

To the person in charge,

This is reproduced in Flame as well.
Please check this issue as soon as possible.

Thank you in advance.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Hi Blake,
Can you check this out?

Comment 3

[Rachelle Yang [:ryang][[e-mail]]]

Hi Blake, would you please share you thoughts on this bug to help us move forward ? 

Thank you very much!

Comment 4

[Rachelle Yang [:ryang][[e-mail]]]

[Blocking Requested - why for this release]:

Comment 5

[Blake Wu [:bwu][:blakewu]]

(In reply to sangyeol from comment #1)
> To the person in charge,
> 
> This is reproduced in Flame as well.
> Please check this issue as soon as possible.
For Flame, users may have small chance to hit this probelm since Flame cannot record qcif video files. 
Unless, user push this kind of video files to phone. 
> Thank you in advance.

Comment 6

[Blake Wu [:bwu][:blakewu]]

Hi John, 
Could you help check if there is any way to fix this probelm from APP side? 
For example, when user choose the "Share" button, stop/unload the current video tag to make it shutdown?

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi Blake,

The email and SMS app use inline activity to receive the share activity now. In the design of window management in System app, we don't receive the visibility change while opening inline activity. That's the reason we don't release the video element.

As per offline discussion, we may use onsuccess and onerror of mozActivity to tell if the inline activity is closed.

If we face a window activity, the onsuccess and onerror will be called immediately and a visibility change event is received. So, using onsuccess and onerror may be a choice to fix it. The flow may be

1. call releaseVideo before creating mozActivity at [1]
2. call restoreVideo after onsuccess or onerror received

And, we may introduce a race condition in "window" activity because we will also receive a visibility change event in this case.

[1] https://github.com/mozilla-b2g/gaia/blob/eb8a140841f2fb2d8f28f45cbfd0bb053d8aa187/apps/video/js/video.js#L623

Comment 8

[Blake Wu [:bwu][:blakewu]]

Hi John, 
Thanks for your help!
I found when user finishes selecting a video, that video will be loaded to get ready to play. Maybe we should not need to do that, just put it to message. If user want to watch it, she/he can click it in the Message APP. Thumbnail should be enough for user to identify which file he/she wants to attach.

Comment 9

[Rachelle Yang [:ryang][[e-mail]]]

Confirmed with partner, launch blocker.

Hi John , per comment 7, would you please kindly check this and provide patch? 
Thank you very much.

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

Rachelle,

This issue may be "wontfix" once the workaround of bug 1088456 landed. Please ask blake for more information.

Comment 11

[Keven Kuo [:kkuo]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #10)
> Rachelle,
> 
> This issue may be "wontfix" once the workaround of bug 1088456 landed.
> Please ask blake for more information.

Is this a duplicate of 1088456?

--
Keven

Comment 12

[vsireesha246]

Hi John,

I tested Bug-1088456 patch.But this issue is not yet resolved.
So would you please recheck In Version2.0

Thanks..
Sireesha

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sireesha,

You are correct. We still need a patch to fix it. The root cause of this issue is the same as bug 1088456., but the behavior is different. The bug 1088456 has one video app index.html frame and one video app view.html frame. And this issue has two video app index.html frame.

The patch of bug 1088456 only let index.html frame know that the view.html frame wants to use hardware decorder. But this issue needs let index.html frame know that another index.html frame wants to use hardware decorder.

David,

This issue is similar to bug 1088456. Would you mind to take it?

Comment 14

[vsireesha246]

Please find the one more STR related to this bug.

1.Open Video app and play any video file
2.Click the options and select Share via SMS
3.SMS app is opened with the video play attached.
4.While in SMS app try to play the attached video file.

Actual:only loading spinner is shown.

Expected:Attached video file should play once we click on it.

Comment 15

[David Flanagan [:djf]]

Changing product, component, and summary to reflect we're handling this as a gaia bug not a gecko bug.

Comment 16

[David Flanagan [:djf]]

Thanks for the analysis John. It does sound very much like 1088456, and we can do a similar fix here.

For master, though, I think that the right way to go is to convert apps that implement the share activity to use window disposition instead of inline.

Comment 17

[David Flanagan [:djf]]

If I fix this like I fixed 1088456, I think I should probably just revert that patch and land one here to fix both bugs.

Comment 18

[Rachelle Yang [:ryang][[e-mail]]]

Hi David, Thanks for your feedback. 

Just to be noted, this is requested by partner as a launch blocker.
Thank you very much!

Comment 19

[vsireesha246]

Attachment: Bug_1085212.patch

Hi David,

Would please provide me the feedback for this workaround.
If this patch is ok..i will upload the PR with areview changes if any.

Thanks..
Sireesha

Comment 20

[David Flanagan [:djf]]

Comment on attachment 8517448 [details] [diff] [review]
Bug_1085212.patch

This is basically the patch that John suggested and what I was thinking of doing at first. Except that I would have updated the comments more. But if you need to land that patch today in the 2.0 tree, I'd be okay with it. (Note that you'd have to uplift 1088456 first, of course.)

But after thinking about this some more, the bigger issue is that if the share activity is inline, then no app that handles the share will be able to play the shared video.  In my patch for bug 1088456, I added code that would pause playback before allowing a share to begin. But that isn't actually enough. I think we need to release the video hardware when a share begins and restore it when the share ends.

We have releaseVideo() and restoreVideo() functions in 2.0, but we just recently removed them from master and 2.1 as part of bug 1082563. If we put these functions back, then we can call releaseVideo() when we start a share, and then call restoreVideo() when the share ends (for the inline share activity case) or when we next become visible (for share activities that are correctly configured to use window disposition.)

If we fix this bug that way, then we don't need to abuse the localStorage mechanism any further because we will never be holding the video hardware when we share a video. Also it will make this patch independent of the patch for 1088456.

Comment 21

[David Flanagan [:djf]]

Once again, John's analysis in comment #7 is very helpful here!

Comment 22

[David Flanagan [:djf]]

Specifically, John says in comment #7 that window disposition activities trigger an onsuccess immediately, so we can safely release/restore without considering visibility at all in that case.

But as John points out, since v2.0 still has release/restore being called on visibility change, we have to look out for race conditions in that release. In 2.1 and master we don't have to worry about that because if we put the release/restore functions back we won't use them on visibility change and will only care about the share activity.

Comment 23

[vsireesha246]

Hi David,

Thank you for your feedback for the patch attached.
Same modifications  applied for V2.0 branch as well and it working fine for all cases.

Coming to your comment#20 says about V2.0 release/restore functions will be called on Share activity case and this change will not use localstorage.
But this change will recreate the Bug-1088456 because view.js don't know about video.js is already playing.So i feel the localstorage or some bridge is needed to communicate between view.js and video.js.

Comment#22 will it really create race conditions..i don't know.
As per my understanding, for Email case both 'storage' and 'visibilitychange' will be fired.
But in your patch(Bug-1088456) will check the document.hidden etc case as well.

So as a summary for this issue Bug-1088456 patch(need to remove the playing check in 'storage' event handler and also playershowing also wont work,not sure) is also required along with my patch.

Please correct me if my analysis is wrong for V2.0 case.

Comment 24

[David Flanagan [:djf]]

vsireesha,

You are correct that bug 1088456 should not have tested for 'playing'. I think that 'playerShowing' will work instead. That was my mistake, and I plan to correct it in the version of this patch that I apply to 2.1 and master. If you end up taking bug 1088456 into your 2.0 tree, then you'll want to make that change as well.

The rest of this patch should be completely independent of 1088456.

Comment 25

[David Flanagan [:djf]]

Attachment: link to patch (for master and 2.1) on github

Another video review for you, Russ. This one prevents hardware contention during inline share activities by resurrecting the release/restore functions that we got rid of recently, but only using them when we share.

It also adds a tweak to the code for the 1088456 patch to make it work correctly even if the video is paused.

I'll need to create a different patch for v2.0 since that branch still has release/restore in use.

Comment 26

[David Flanagan [:djf]]

Comment on attachment 8518579 [details] [review]
link to patch (for master and 2.1) on github

vsireesha: does this patch look to you like it will work?  It won't apply on the v2.0 branch, but I'll create a version that does.  Feel free to test on v2.1 or on master, if you can.

Comment 27

[vsireesha246]

Attachment: Bug_1085212_V2.0.patch

Hi David,

Sorry i cant test your patch in Master and V2.1.But i checked the code and it looks fine for me.
I tried to make Patch for V2.0 and tested in V2.0 code and works fine.
So please check and let me know your feedback.

Thanks..
Sireesha

Comment 28

[David Flanagan [:djf]]

Comment on attachment 8518893 [details] [diff] [review]
Bug_1085212_V2.0.patch

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

I see two problems with this patch. 

1) It includes all code from 1088456. I don't think that is necessary to fix this bug, and if we want to handle that bug we should separately make that uplift decision in 1088456.

2) it includes the new needsRestore variable, but never sets it. Since this version of the code still uses release/restore on visibility change, we need this flag to prevent the video from being released twice or restored twice.  In particular this is a concern for share activities that use window disposition. We don't want to break those while fixing the inline activity case.

Comment 29

[David Flanagan [:djf]]

Attachment: Link to v2.0 patch on github

Russ,

This version of the patch is different because the 2.0 branch still has the old releaseVideo/restoreVideo code available. Also, I've taken part of the 1088456 patch and included it here: just the part where we pause playback when the menu is shown. I'm not doing anything with the localStorage hack in this bug. (That is a separate and more obscure bug and its uplift decision should be separate from this one.)

vsireesha: this patch works for me and fixes the pick and view bugs described in comment #0 and comment #14. Do you agree that this patch is sufficient to resolve this bug?

Comment 30

[Russ Nicoletti [:russn]]

Comment on attachment 8519080 [details] [review]
Link to v2.0 patch on github

Looks good.

Comment 31

[David Flanagan [:djf]]

Comment on attachment 8519080 [details] [review]
Link to v2.0 patch on github

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 #): not a regression, just a bug that we never noticed before. Or maybe a regression caused by window management changes for 1.3T to ensure that apps that initiate inline activities don't get OOMs.

[User impact] if declined: Important video->share->sms->view/pick->video activity flows will be broken with no error messages.

[Testing completed]: yes

[Risk to taking this patch] (and alternatives if risky): not very risky. The patch uses existing code to release and reclaim the video hardware and just calls it before and after the share activity.

[String changes made]: none

Comment 32

[Russ Nicoletti [:russn]]

Comment on attachment 8518579 [details] [review]
link to patch (for master and 2.1) on github

r+, David is making a minor name change to a newly introduced variable to make the code more understandable/readable/intuitive.

Comment 33

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/62da4f28779828d8224ae4aecc9a428a5e80dca7

Comment 34

[David Flanagan [:djf]]

Comment on attachment 8518579 [details] [review]
link to patch (for master and 2.1) on github

[Approval Request Comment] See comment #31, where I make the case for uplifting to 2.0. This bug affects 2.1 as well, and seems at least as likely (if not more so) to occur than 1079519 which was 2.1+ and had a risker patch.

Comment 35

[vsireesha246]

Comment on attachment 8519080 [details] [review]
Link to v2.0 patch on github

Hi David,

I tested this patch in V2.0 and working fine.
But the Bug-1088456(with this patch) still reproducible in V2.0.
So we need to uplift or separate patch to resolve Bug-1088456 for V2.0..

Thanks..
Sireesha

Comment 36

[bhavana bajaj [:bajaj]]

Comment on attachment 8519080 [details] [review]
Link to v2.0 patch on github

Requesting QA verification and some exploratory testing once this lands on branches.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/3d3575dfb1886f44ec0ada340cd588b7c22af3dd
v2.0: https://github.com/mozilla-b2g/gaia/commit/ef49225b0062d9d4bf4185371ae40f254c189ada

Comment 38

[sarsenyev]

Verified the issue is fixed on Flame 2.2, 2.1 and 2.0
Able to add multiple videos to the Message attachment

Device: Flame 2.2 Master KK
BuildID: 20141111040202
Gaia: 6af3a8a833eb8bb651e8b188cb3f3c3a43bb4184
Gecko: 76b85b90a8cb
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1 KK
BuildID: 20141111001201
Gaia: 7154f9aa0a69af56c8bd89be0c98d9791449527b
Gecko: 452db1a0c9a0
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
  
Device: Flame 2.0 KK
BuildID: 20141111000205
Gaia: dfdd6268b9784eafdd509f0ddf71407698ed5080
Gecko: fcc5d31f84d7
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0