1079519 - Unable to play video clips during WEBRTC video call

Status: RESOLVED FIXED

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

Description

[Jay]

[Blocking Requested - why for this release]:

Steps to reproduce:
1. Establish video call between 8x10 devices through Loop client app
2. During video call select Home option
3. Now launch Videos app and try to play any video clip

Issue:
Observed unable to play the selected video clip

Expected behavior:
Should able to play any video clip during WEBRTC video call

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

There is only one hardware decoder; in a webrtc call (h.264 at least) it's in use.  If the Video player can fall back on software decoding then it should work.

(If you play a video first, then Webrtc should fall back to VP8 in software for video calls.)

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Adding qawanted to see if we can reproduce this on Flame and for branch checks.

Comment 3

[Seth Miko [:SMiko]]

Unable to get branch checks as this issue does NOT repro on Flame 2.1 (319mb)

Actual result: Video clips play during WEBRTC video call.

Device: Flame 2.1
BuildID: 20141012001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 4

[Maire Reavy [:mreavy]]

Jay -- Can you resolve with Seth the conflict between your initial report and Seth's results in Comment 3?

If a bug does exist, it wouldn't be in the Loop app.  It would be in Video/Audio or the Video app that isn't falling back to use the software decoder -- or informing the user that the H/W decoder is already in use.

Comment 5

[Jay]

(In reply to Seth Miko [:SethM] from comment #3)
> Unable to get branch checks as this issue does NOT repro on Flame 2.1 (319mb)
> 
> Actual result: Video clips play during WEBRTC video call.
> 
> Device: Flame 2.1
> BuildID: 20141012001201
> Gaia: d18e130216cd3960cd327179364d9f71e42debda
> Gecko: 610ee0e6a776
> Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
> Version: 34.0a2 (2.1)
> Firmware: V180
> User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Seth,
Which video format did you try? Can you try the video with H.264?

Comment 6

[Seth Miko [:SMiko]]

(In reply to Jay from comment #5)
> Seth,
> Which video format did you try? Can you try the video with H.264?

We were able to view gallery videos. 3gpp format. 

Jayme, Can you try the video with H.264?

Comment 7

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

I was unable to reproduce this issue using an H.264 encoded video either.

Environmental Variables:
Device: Flame 2.1
BuildID: 20141015040132
Gaia: 82565292c0fa86e3ddcda290cae7f5cb702f654c
Gecko: d8d578cb569c
Version: 34.0 (2.1) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Loop version: a364326

Comment 8

[Diego Wilson [:diego]]

(In reply to Jayme Mercado [:JMercado] from comment #7)
> I was unable to reproduce this issue using an H.264 encoded video either.
> 
> Environmental Variables:
> Device: Flame 2.1
> BuildID: 20141015040132
> Gaia: 82565292c0fa86e3ddcda290cae7f5cb702f654c
> Gecko: d8d578cb569c
> Version: 34.0 (2.1) 
> Firmware Version: v180
> User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
> Loop version: a364326

Jayme,

Do you mean to say that you can play H.264 videos during a Loop video call?

Comment 9

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Yes I can.

Comment 10

[Diego Wilson [:diego]]

Are you making your own gecko build? If so, you may need to explicitly enable WebRTC H.264:

https://bugzilla.mozilla.org/show_bug.cgi?id=1073025#c10

I've been trying to get this pref enabled by default in that bug.

Comment 11

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

No.  The builds used are from the pvt website.

Comment 12

[Diego Wilson [:diego]]

(In reply to Jayme Mercado [:JMercado] from comment #11)
> No.  The builds used are from the pvt website.

Randell,

Do you know if those builds have WebRTC H.264 enabled?

Comment 13

[Andrew Overholt [:overholt]]

CAF 2.1 FC blocker

Comment 14

[Jay]

(In reply to Jayme Mercado [:JMercado] from comment #11)
> No.  The builds used are from the pvt website.
Can you confirm if h264 is enabled for WebRTC call on your build?

You can pull the user.js file with the following adb command from the target
- adb pull /system/b2g/defaults/pref/user.js

and check if you see this line in the user.js. 
- pref('media.peerconnection.video.h264_enabled', true);

If not, you can add the above line and push it back to the target and restart the b2g with the following command.

adb push user.js /system/b2g/defaults/pref/user.js
adb shell chmod 777 /system/b2g/defaults/pref/user.js
adb shell stop b2g
adb shell start b2g

Comment 15

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Using the same build and loop client as yesterday I added that line as requested in comment 14 and this issue still did not repro.

Environmental Variables:
Device: Flame 2.1
BuildID: 20141015040132
Gaia: 82565292c0fa86e3ddcda290cae7f5cb702f654c
Gecko: d8d578cb569c
Version: 34.0 (2.1) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Loop version: a364326

Comment 16

[Jay]

(In reply to Jayme Mercado [:JMercado] from comment #15)
> Using the same build and loop client as yesterday I added that line as
> requested in comment 14 and this issue still did not repro.
> 
> Environmental Variables:
> Device: Flame 2.1
> BuildID: 20141015040132
> Gaia: 82565292c0fa86e3ddcda290cae7f5cb702f654c
> Gecko: d8d578cb569c
> Version: 34.0 (2.1) 
> Firmware Version: v180
> User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
> Loop version: a364326

I just tried again here and I still see the problem. 
Can you enable the webRTC log "NSPR_LOG_MODULES=signaling:6,WebrtcOMXH264VideoCodec:6,timestamp" and see if you see the following message?

[main|WebrtcVideoSessionConduit] VideoConduit.cpp:528: ConfigureSendMediaCodec for H264_P1

This confirms that your build was using H.264 codec instead of VP8

Comment 17

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

(In reply to Jay from comment #16)
> (In reply to Jayme Mercado [:JMercado] from comment #15)
> > Using the same build and loop client as yesterday I added that line as
> > requested in comment 14 and this issue still did not repro.
> > 
> > Environmental Variables:
> > Device: Flame 2.1
> > BuildID: 20141015040132
> > Gaia: 82565292c0fa86e3ddcda290cae7f5cb702f654c
> > Gecko: d8d578cb569c
> > Version: 34.0 (2.1) 
> > Firmware Version: v180
> > User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
> > Loop version: a364326
> 
> I just tried again here and I still see the problem. 
> Can you enable the webRTC log
> "NSPR_LOG_MODULES=signaling:6,WebrtcOMXH264VideoCodec:6,timestamp" and see
> if you see the following message?
> 
> [main|WebrtcVideoSessionConduit] VideoConduit.cpp:528:
> ConfigureSendMediaCodec for H264_P1
> 
> This confirms that your build was using H.264 codec instead of VP8

I'm not sure what you want me to do.

Comment 18

[Jay]

(In reply to Jayme Mercado [:JMercado] from comment #17)
> (In reply to Jay from comment #16)
> > (In reply to Jayme Mercado [:JMercado] from comment #15)
> > > Using the same build and loop client as yesterday I added that line as
> > > requested in comment 14 and this issue still did not repro.
> > > 
> > > Environmental Variables:
> > > Device: Flame 2.1
> > > BuildID: 20141015040132
> > > Gaia: 82565292c0fa86e3ddcda290cae7f5cb702f654c
> > > Gecko: d8d578cb569c
> > > Version: 34.0 (2.1) 
> > > Firmware Version: v180
> > > User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
> > > Loop version: a364326
> > 
> > I just tried again here and I still see the problem. 
> > Can you enable the webRTC log
> > "NSPR_LOG_MODULES=signaling:6,WebrtcOMXH264VideoCodec:6,timestamp" and see
> > if you see the following message?
> > 
> > [main|WebrtcVideoSessionConduit] VideoConduit.cpp:528:
> > ConfigureSendMediaCodec for H264_P1
> > 
> > This confirms that your build was using H.264 codec instead of VP8
> 
> I'm not sure what you want me to do.
We are suspecting that your WebRTC session is using VP8 video codec instead of H.264 so by looking at the webRTC log, we can isolate if this is the reason that you don't see the problem.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Modify /system/bin/b2g.sh to include "export NSPR_LOG_MODULES=signaling:6,WebrtcOMXH264VideoCodec:6,timestamp" near the start/other exports.

adb pull /system/bin/b2g.sh .
edit b2g.sh per above
adb push b2g.sh /system/bin
adb shell chmod 0777 /system/bin/b2g.sh
adb shell stop b2g
adb shell start b2g

Then test and capture logs with adb logcat

Comment 20

[Maire Reavy [:mreavy]]

Hi Bhavana, During our TEF/QC call today, we discussed this bug, and we all agree that this is not a Loop bug or a WebRTC bug.  It's actually a video app bug.  The video app should either fallback to using a software decoder (if that's possible) or it should pop up a message to the user that it can't play the H.264 video (due to the hardware already being in use).

I'm not sure who to transfer this to since it's a video app issue;  so I'm asking for your help in finding the right owner.  Thanks!

Comment 21

[bhavana bajaj [:bajaj]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #20)
> Hi Bhavana, During our TEF/QC call today, we discussed this bug, and we all
> agree that this is not a Loop bug or a WebRTC bug.  It's actually a video
> app bug.  The video app should either fallback to using a software decoder
> (if that's possible) or it should pop up a message to the user that it can't
> play the H.264 video (due to the hardware already being in use).
> 
> I'm not sure who to transfer this to since it's a video app issue;  so I'm
> asking for your help in finding the right owner.  Thanks!

Starting with mikeh/Hema here to get input on Maire's investigation here..

Comment 22

[Mike Habicher [:mikeh] (high bugzilla latency)]

djf and/or sotaro might know more about how to address this.

Comment 23

[Mike Habicher [:mikeh] (high bugzilla latency)]

One question: are we able to fall back to a s/w codec for all formats, in the case where the h/w engine is busy?

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #23)
> One question: are we able to fall back to a s/w codec for all formats, in
> the case where the h/w engine is busy?

As a b2g product point of view, it is not acceptable to use s/w codec on b2g. s/w codec does not provide enough quality as a product from decoding performance and profile support point of view. In early day's of b2g v1.01, H.264 video's sw decoding was partially enabled, but it is disabled from memory resource and performance point of view. And since b2g v1.4 VP8 video hw decoding is enabled by Bug 986381, because sw codec does not provide enough quality. 

And during webRTC, normally majority of cpu time is consumed by WebRTC related task and it uses a lot of memory, b2g's normal cpu do not have enough resource to handle both ebRTC and sw video decoding concurrently. Forcibly enable sw codec could cause another problems.

Comment 25

[Sotaro Ikeda [:sotaro]]

In current gecko, WebRTC does not release the resources until it releases them. If we want fix this problem, WebRTC needs a way to release their resources in some use cases.

About video playback, video hw codec is released when application go to background or video tag is unbound from the tree, they are implemented by Bug 871485 and Bug 1029552.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> In current gecko, WebRTC does not release the resources until it releases
> them. If we want fix this problem, WebRTC needs a way to release their
> resources in some use cases.

We can't release them with the current app behavior, which is that calls continue if the app goes to the background (so a user can look up (say) a map location or contact, and tell the other person in the call).

With a fair bit of extra logic we could (in theory) release the decoder when backgrounded (but continue encoding) as decoded video is not visible anyways, and then when getting back to foreground re-acquire the decoder, and then force an "decode error" to force error recovery (IDR).  This is not feasible to complete for 2.1.

WebRTC does release the codec whenever not in a call or attempting to create a call.

> About video playback, video hw codec is released when application go to
> background or video tag is unbound from the tree, they are implemented by
> Bug 871485 and Bug 1029552.

Does this mean a browser tab with an h.264 video in it will block webrtc from getting an H.264 HW decoder?  Does the state of the <video> element matter?  (loaded but not started; paused; completed)?

It sounds like the only short-term/safe option is to have the Video app cleanly handle failure to reserve the H.264 decoder.

Comment 27

[Sotaro Ikeda [:sotaro]]

> > About video playback, video hw codec is released when application go to
> > background or video tag is unbound from the tree, they are implemented by
> > Bug 871485 and Bug 1029552.
> 
> Does this mean a browser tab with an h.264 video in it will block webrtc
> from getting an H.264 HW decoder?

h.264 video playback does not block webrtc, but webrtc fails to get h.264 and fallback to VP8. It is implemented by Bug 1003712.

>  Does the state of the <video> element matter?  (loaded but not started; paused; completed)?

If video is at least loaded and application is in shown state, hw video decoder is acquired by video tag. Therefore, in stated, paused and completed state, the video tag acquire hw video codec.

> 
> It sounds like the only short-term/safe option is to have the Video app
> cleanly handle failure to reserve the H.264 decoder.

Comment 28

[Sotaro Ikeda [:sotaro]]

In current implementation, when video tag tries to acquire hw video decoder and the hw video decoder is already acquired by others, the video tag wait until it acquires hw video decoder or it ends to request to acquire the codec. Such video tag's behavior is implemented to handle contention between video tags in same application and contention between application.

In android, video codec release in VideoView during application switch happens synchronously. In android, application switch is controlled synchronously by WindowManagerService. New application becomes foreground after previous application's releasing resource phase end. 

But in b2g, such application's video codec's release happens asynchronously. Therefore, such wait becomes necessary. To handle contention between video tags. b2g's video tags hw video decoder sharing is done by implicitly. It expects when application becomes background, the video decoder is released. But in WebRTC case, video decoder is not released. Therefore it might be better that video tag doe not wait hw video codec acquire when the video decoder is acquired by WebRTC.

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

If the App or browser releases the HW resource when backgrounded (or in the browser if the tab isn't visible), then I think we're good on that front, and WebRTC will be able to reserve the codecs when it's in the foreground/focused.

A *future* usecase for maintaining decode while switching to another app would be to have the webrtc app thumbnail the remote video when you switch to another app - you'd see the other person, but smaller and you'd be able to move the remote video around to get to whatever parts of the app you need to see (lookup maps, restaurant info, contacts, web stuff, etc).  This doesn't affect the discussion about today, and I think my comment above stands: It sounds like the only short-term/safe option is to have the Video app cleanly handle failure to reserve the H.264 decoder (perhaps an error message, perhaps generic error if we can't change the strings immediately).

Per the asynchronous/wait issue: handling that differently depending on who owns it would work; another option would be to time out the wait.

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> But in WebRTC case, video decoder is not released. Therefore it might be
> better that video tag doe not wait hw video codec acquire when the video
> decoder is acquired by WebRTC.

Since video playback during WebRTC seems a product requirement, it could not become a choice.

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Randell Jesup [:jesup] from comment #29)
> 
> Per the asynchronous/wait issue: handling that differently depending on who
> owns it would work; another option would be to time out the wait.

Yhea, timeout wait seems to becomes necessary especially when WebRTC is going to acquire hw video decoder.

Comment 32

[Sotaro Ikeda [:sotaro]]

Another choice for fixing the problem is adding multiple video decoder support. In low end device, only one instance of video codec is supported. Therefore, current b2g implementation try to limit at most one video decoder instance concurrently. But middle or higher range devices have multiple hw video decoder support. The problem of supporting multiple video decoder is that the device could have some device specific limitation. And the limitation is not explicitly exposed to android frameworks and also to b2g.

One example of limitation: In a device, 4 video decoder could be instantiated for 4 VGA videos. But only one video decoder could be instantiated for full HD video. On some devices, video decoder's number is limited by total video buffers' size.

In WebRTC, video decoding size could be limited by a b2g device. It is different from supporting concurrent multiple any size videos playback. If the device ensure that one video decoder(limited sized video) for WebRTC and one video decoder for other uses(any sized video). We could fix this problem by mimimum change.

Comment 33

[Sotaro Ikeda [:sotaro]]

By the way, the limitation of video decoder instance number is necessary also from stability point of view. Some web sites could have multiple video tags. If b2g browser tab start to instantiate multiple sw video decoders, the browser tab could be easily crashed/or killed because of using too much memory.

Comment 34

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

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> Another choice for fixing the problem is adding multiple video decoder
> support. In low end device, only one instance of video codec is supported.
> Therefore, current b2g implementation try to limit at most one video decoder
> instance concurrently. But middle or higher range devices have multiple hw
> video decoder support. The problem of supporting multiple video decoder is
> that the device could have some device specific limitation. And the
> limitation is not explicitly exposed to android frameworks and also to b2g.
> 
> One example of limitation: In a device, 4 video decoder could be
> instantiated for 4 VGA videos. But only one video decoder could be
> instantiated for full HD video. On some devices, video decoder's number is
> limited by total video buffers' size.
see also Bug 978672. 
> 
> In WebRTC, video decoding size could be limited by a b2g device. It is
> different from supporting concurrent multiple any size videos playback. If
> the device ensure that one video decoder(limited sized video) for WebRTC and
> one video decoder for other uses(any sized video). We could fix this problem
> by mimimum change.
see also Bug 977438

Comment 35

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

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> Another choice for fixing the problem is adding multiple video decoder
> support. In low end device, only one instance of video codec is supported.
> Therefore, current b2g implementation try to limit at most one video decoder
> instance concurrently. But middle or higher range devices have multiple hw
> video decoder support. The problem of supporting multiple video decoder is
> that the device could have some device specific limitation. And the
> limitation is not explicitly exposed to android frameworks and also to b2g.
Yes. 
A quick solution/workaround could be to add more instance by changing VIDEO_DECODER_COUNT.  
http://dxr.mozilla.org/mozilla-central/source/content/media/omx/mediaresourcemanager/MediaResourceManagerService.h#35

Comment 36

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #35)
> (In reply to Sotaro Ikeda [:sotaro] from comment #32)
> > Another choice for fixing the problem is adding multiple video decoder
> > support. In low end device, only one instance of video codec is supported.
> > Therefore, current b2g implementation try to limit at most one video decoder
> > instance concurrently. But middle or higher range devices have multiple hw
> > video decoder support. The problem of supporting multiple video decoder is
> > that the device could have some device specific limitation. And the
> > limitation is not explicitly exposed to android frameworks and also to b2g.
> Yes. 
> A quick solution/workaround could be to add more instance by changing
> VIDEO_DECODER_COUNT.  
> http://dxr.mozilla.org/mozilla-central/source/content/media/omx/
> mediaresourcemanager/MediaResourceManagerService.h#35
But this may only work for QC platforms. 
AFAIK, currently QC platforms with FFOS inside should be able to support 4 instances. 
But we still need a way to know how many instance on different platforms. 
I am planning to request a session to discuss HW codec resource management in the Portland workweek.

Comment 37

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #35)
> (In reply to Sotaro Ikeda [:sotaro] from comment #32)
> > Another choice for fixing the problem is adding multiple video decoder
> > support. In low end device, only one instance of video codec is supported.
> > Therefore, current b2g implementation try to limit at most one video decoder
> > instance concurrently. But middle or higher range devices have multiple hw
> > video decoder support. The problem of supporting multiple video decoder is
> > that the device could have some device specific limitation. And the
> > limitation is not explicitly exposed to android frameworks and also to b2g.
> Yes. 
> A quick solution/workaround could be to add more instance by changing
> VIDEO_DECODER_COUNT.  
> http://dxr.mozilla.org/mozilla-central/source/content/media/omx/
> mediaresourcemanager/MediaResourceManagerService.h#35

Changing VIDEO_DECODER_COUNT means, the devide support VIDEO_DECODER_COUNT of hw codecs for any sized video that is supported by the devide.

Comment 38

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #36)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #35)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #32)
> > > Another choice for fixing the problem is adding multiple video decoder
> > > support. In low end device, only one instance of video codec is supported.
> > > Therefore, current b2g implementation try to limit at most one video decoder
> > > instance concurrently. But middle or higher range devices have multiple hw
> > > video decoder support. The problem of supporting multiple video decoder is
> > > that the device could have some device specific limitation. And the
> > > limitation is not explicitly exposed to android frameworks and also to b2g.
> > Yes. 
> > A quick solution/workaround could be to add more instance by changing
> > VIDEO_DECODER_COUNT.  
> > http://dxr.mozilla.org/mozilla-central/source/content/media/omx/
> > mediaresourcemanager/MediaResourceManagerService.h#35
> But this may only work for QC platforms. 
> AFAIK, currently QC platforms with FFOS inside should be able to support 4
> instances. 
> But we still need a way to know how many instance on different platforms. 
> I am planning to request a session to discuss HW codec resource management
> in the Portland workweek.

If it is supported by the device, the supported hardware works without problem. If it is not supported by a hardware it just work as fallback way.

Comment 39

[David Flanagan [:djf]]

I don't follow the low-level back and forth here, so I'm not sure if there is a possible platform fix here.

If not, I think the best we can do is have the Video app detect when it can't play videos and display a message to the user. If we need to do that, please assign this bug to :russn.

Sotaro: how can we tell in JS when the video hardware is not available? Is there an error of some sort, or do we just have to wait? If we don't get a "canplay" event within 1000ms of setting the src attribute of the video tag can we assume that the hardware is not available and display a message to the user until we do get that event?

If we are going to fix the video app by just displaying a message to the user, we'll need UX input and it will involve late l10n, so setting late-l10n in the whiteboard.

Stephanie, is there someone from UX who can help us determine what the message should be?

If we end up having to go that route, it will obviously involve some kind of late l10n so setting that in the whiteboard.

Comment 40

[:shell escalante]

based on comments 26 & 29 & 33 - it looks like failing gracefully is the best/only 2.1 option.  please correct me if i missed some ideas along the way - so assigning to :russn based on comment 39 for graceful failure.

Comment 41

[Sotaro Ikeda [:sotaro]]

(In reply to David Flanagan [:djf] from comment #39)
> Sotaro: how can we tell in JS when the video hardware is not available?

Hmm, there seems no way to tell JS side about no video hardware is available. In current gecko, video element just wait until hardware is acquired.

> Is there an error of some sort, or do we just have to wait?

I checked the following, but there seems no such event nor error.
https://developers.whatwg.org/the-video-element.html#the-video-element

> If we don't get a"canplay" event within 1000ms of setting the src attribute of the video tag
> can we assume that the hardware is not available and display a message to
> the user until we do get that event?

It seems dangerous to be used in product code.

Comment 42

[Sotaro Ikeda [:sotaro]]

cpearce, can you comment to comment 39?

Comment 43

[Russ Nicoletti [:russn]]

Jay and Jaymie, how can I get the Loop app? I looked in marketplace but I couldn't find it there. Thanks.

Comment 44

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

(In reply to David Flanagan [:djf] from comment #39)
> I don't follow the low-level back and forth here, so I'm not sure if there
> is a possible platform fix here.
> 
> If not, I think the best we can do is have the Video app detect when it
> can't play videos and display a message to the user. If we need to do that,
> please assign this bug to :russn.
> 
> Sotaro: how can we tell in JS when the video hardware is not available? Is
> there an error of some sort, or do we just have to wait? If we don't get a
> "canplay" event within 1000ms of setting the src attribute of the video tag
> can we assume that the hardware is not available and display a message to
> the user until we do get that event?

There is currently no way in the video API to tell whether a video element is not able to play because the HW decoder is already engaged. Potentially we could add a moz specific event to signal this.

However, I don't know if the WebRTC stack uses the same HW codec sharing code that the <video> code uses, so it may require some extra plumbing to get these two systems communicating. i.e. it may not be a trivial small patch.

Setting a timeout could be error prone, if you're loading a large video. It's probably the easiest solution for now. You probably want to set a timeout that is canceled when the "loadeddata" event is fired.


> 
> If we are going to fix the video app by just displaying a message to the
> user, we'll need UX input and it will involve late l10n, so setting
> late-l10n in the whiteboard.
> 
> Stephanie, is there someone from UX who can help us determine what the
> message should be?
> 
> If we end up having to go that route, it will obviously involve some kind of
> late l10n so setting that in the whiteboard.

Comment 45

[David Flanagan [:djf]]

Shell: Hema tells me that it is too late for me to have set the late-l10n flag on this bug. If we're going to do some kind of fail gracefully solution, I don't know how we can do that without localized text. Do you have any negotiating power with the localization team? Or can you get a visual designer to create a language-independent icon that conveys the message "you can't play your video now because you're on a webrtc call".

I suppose we could do some kind of hack where we have the loop app implement a "return-to-call-in-progress" web activity. Then when the video app is unable to play a video it tries to punt by invoking that activity and taking the user back to their call. That's very counterintuitive UX, but it would avoid l10n.

Comment 46

[Stephany Wilkes]

Given the markets we're shipping in for this release, I don't see how we can avoid L10N here. The visual design workaround due to time constraints is not ideal and too great. 

I'm not sure what the remaining UX question is: 

1) Is it for message copy to follow the video app detecting when it can't play videos and display a message to the user (which I think is the ideal behavior here, as things should "just work" and, barring that, tell the user why they don't work and how to fix it); or

2) Is it about failing gracefully if the video app can't detect and/or display a message (which is a problem that should be fixed regardless)? If so, what exactly does failing gracefully look like here? I'm having trouble understanding what the user would see in that scenario.

Comment 47

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

https://github.com/mozilla-b2g/firefoxos-loop-client
Download the zip and then you can push it to the phone using firefox nightly's app manager.

Comment 48

[Russ Nicoletti [:russn]]

In IRC discussion with Stephany, she reiterated the only feasible workaround involves displaying a useful message to the user about what is happening. I will begin implementing the workaround in the video app of displaying *some* message to the user when a video doesn't start playing after some predetermined timeout (and then to hide the message if the video does begin to play).

Comment 49

[Stephany Wilkes]

I am trying to see if we can use an existing string here. Also flagging Jacqueline since she owns UX for this area.

Comment 50

[Axel Hecht [:Pike]]

I don't really understand what this bug is about. The way I grok it, is this:

If the youtube app plays first, it's getting the hardware decoder.
If the Hello app plays first, it's getting the hardware decoder.
If the video app plays first, it's getting the hardware decoder.
If the my-awesome-game-intro-video plays first, it's getting the hardware decoder.

Anything that tries to play a video afterwards that would get hardware decoding is playing a gray block?

Comment 51

[Stephany Wilkes]

Sorry, I meant to flag Juwei here, as I did on the other similar bug.

Comment 52

[Russ Nicoletti [:russn]]

Attachment: bug-1079519.diff

This is the diff for a WIP fix (workaround) for when the video app cannot load a video within a "reasonable" (one second but this is tbd) amount of time.

David, can you take a look and give me feedback?

Comment 53

[Hema Koka [:hema]]

Its late for new strings.

Suggestion seems to be to use generic video-unsupported or video-unknown error as the workaround for 2.1:

error-unsupported = Video cannot be played: either because the server or network failed or because the format is not supported.
error-unknown = An unknown video error occurred.

(Lets get a proper message in for master and also file a bug for a permanent fix based on earlier suggestions from Chris and others. The message can be removed once we have the platform fix)

Comment 54

[Juwei Huang[:juwei]]

Ni? Mike instead since the UX person will be re-assign.

Comment 55

[Russ Nicoletti [:russn]]

Regarding which existing error message to use, IMO 'error-unknown' is better than 'error-unsupported' because 'error-unsupported' is more misleading to the user. 'error-unknown' is not great but is less misleading.

Comment 56

[Hema Koka [:hema]]

We need to wrap up these bugs today. Stephany as the product owner please make the call on error message for 2.1 (waiting another day for Taipei to come online, will delay this bug). 

My choice is the generic "error-unknown" as discussed over IRC. 

Thanks
Hema

Comment 57

[Stephany Wilkes]

Use error-unknown, please. This is what I agreed with Hema in IRC so let's do it. Thanks!

Comment 58

[Stephany Wilkes]

Also - noting for a 2.2 improvement to use a new string. The generic error message is not ideal but it's better than the alternatives for 2.1.

Comment 59

[Stephany Wilkes]

Removing flag for Mike since we have a workaround.

Comment 60

[Russ Nicoletti [:russn]]

Attachment: Github PR: https://github.com/mozilla-b2g/gaia/pull/25627

This is a PR for displaying the "unknown-error" message when a video is not loaded within a predetermined amount of time (currently one second).

Comment 61

[David Flanagan [:djf]]

Comment on attachment 8513693 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25627

Russ,

I left a few comments on github asking for minor code cleanup.

I think you may also need to add the same fix to view.js, don't you?

Also, please check what happens if you start a video in the video app, then go to the homescreen, and then start a webrtc call, and then return to the video app. I think that this patch will not correctly handle that case.

If not, then I think maybe you need to write a ensurePlaybackStarts() function. Call it when the video.src property is first set, and also call it every time that play() is called on the video. This function could probably listen for a 'playing' event. And, if you structure it with nested functions, I think that the timeout id could be a local variable instead of a global. If you put it in a new file, then you can use it for both the regular and the view activity case.

Comment 62

[David Flanagan [:djf]]

It sucks that we're scrambling here to fix a last minute regression caused by the Loop app. Ideally, Loop would be smart enough to automatically switch from audio+video to audio only when either side of the conversation goes to the background. I suppose in a multi-way call, that can't be done. Does WebRTC multiplex the audio and the video streams into a single video format so that the video hardware is required to play the audio? If so, then maybe this really isn't fixable in any good way in Loop.

This same bug will affect 3rd party apps and web pages that try to play video, so a gecko patch would really be a better solution here, if we could do it somehow.  Is there some way we could make video element display a "can't play your video now because the hardware is in use, please wait" icon when this happens, that would be a more universal solution.

I wonder if we can hack the video controls chrome xul file (whereever it lives) to give us this feature, using the same basic algorithm that we use in Russ's patch here.

Comment 63

[David Flanagan [:djf]]

Russ,

Also, if you're doing this as a patch for master, go ahead and define the desired new "video-hardware-in-use' string in the locale file.

Then, when we create a version of the patch for uplift, redefine that new string in terms of an existing string. So for 2.1, you'd have a line in the locale file like:

video-hardware-in-use-title = {{error-unknown}}
video-hardware-in-use-text = 

I think that is the most elegant way to handle it.

Comment 64

[Francesco Lodolo [:flod]]

(In reply to David Flanagan [:djf] from comment #63)
> video-hardware-in-use-title = {{error-unknown}}
> video-hardware-in-use-text = 

FYI that still counts as adding strings.

Comment 65

[David Flanagan [:djf]]

(In reply to Francesco Lodolo [:flod] from comment #64)
> (In reply to David Flanagan [:djf] from comment #63)
> > video-hardware-in-use-title = {{error-unknown}}
> > video-hardware-in-use-text = 
> 
> FYI that still counts as adding strings.

Well that sucks. No translation would be required, but I suppose I can see how it would cause problems for the l10n toolchain.

Comment 66

[David Flanagan [:djf]]

So the XUL video controls file is mozilla-central/toolkit/content/widget/videocontrols.xul  Justin Dolske reviewed the last two changes to it. Maybe he could tell us whether we can fix this bug in that file.

Justin: can we modify videocontrols.xml (just for B2G) so that if a video does not being to play promptly, we display some kind of "hardware in use" or "please wait" error message to the user? This is for FirefoxOS 2.1, so it would have to be done without string changes.  Obviously the reason I'm asking is that we want to fix this issue for all <video> elements, including in web pages and 3rd party apps. It is not clear to me whether any part of videocontrols.xml can be visible for video elements that do not have the controls attribute specified.

Comment 67

[Justin Dolske [:Dolske]]

(In reply to David Flanagan [:djf] from comment #66)

> Justin: can we modify videocontrols.xml (just for B2G) so that if a video
> does not being to play promptly, we display some kind of "hardware in use"
> or "please wait" error message to the user? This is for FirefoxOS 2.1, so it
> would have to be done without string changes.

Those two sentences are contradictory, so I'm not sure what you're asking.

The default video controls do have a few existing strings for errors:

http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/videocontrols.dtd?force=1#26

The error.generic string gives you "Video playback aborted due to an unknown error". If the media backend is adjusted to throw an error event when no decoder is available, that should work.

If you absolutely need a specific error without string changes, I suppose you could hard-code an English-only string. 

> Obviously the reason I'm
> asking is that we want to fix this issue for all <video> elements, including
> in web pages and 3rd party apps. It is not clear to me whether any part of
> videocontrols.xml can be visible for video elements that do not have the
> controls attribute specified.

The default controls do not exist on video elements that provide their own controls. In fact B2G hard-disabled that case due for performance (bug 821695). But one would (optimistically) hope that the page's own custom controls are handling error events, so firing an error when we can't get a decoder should trigger the page's own generic error handling UI.

Comment 68

[David Flanagan [:djf]]

Russ: the latest word on the video bug is that we should go ahead with a text-based error message. It won't be localized in time, but we're going to land it anyway, and partners can cherry pick localizations into their trees when it gets localized in master.  So please prepare a patch for master using fully localized text saying something like "Cannot play video: Another app is currently playing a video." This is what Justin has done for bug 1079543. His patch for master is in 1080677, and it looks like they're just going to uplift that to 2.1 and leave it unlocalized for now.

Meanwhile, perhaps there is something we can do in videocontrols.xml to address this issue in other contexts, but that can happen in parallel with this bug.

Comment 69

[David Flanagan [:djf]]

(In reply to Justin Dolske [:Dolske] from comment #67)
> (In reply to David Flanagan [:djf] from comment #66)
> 
> > Justin: can we modify videocontrols.xml (just for B2G) so that if a video
> > does not being to play promptly, we display some kind of "hardware in use"
> > or "please wait" error message to the user? This is for FirefoxOS 2.1, so it
> > would have to be done without string changes.
> 
> Those two sentences are contradictory, so I'm not sure what you're asking.

Yeah, after I asked that question I learned that gecko doesn't have localized strings.

> > Obviously the reason I'm
> > asking is that we want to fix this issue for all <video> elements, including
> > in web pages and 3rd party apps. It is not clear to me whether any part of
> > videocontrols.xml can be visible for video elements that do not have the
> > controls attribute specified.
> 
> The default controls do not exist on video elements that provide their own
> controls. In fact B2G hard-disabled that case due for performance (bug
> 821695). But one would (optimistically) hope that the page's own custom
> controls are handling error events, so firing an error when we can't get a
> decoder should trigger the page's own generic error handling UI.

Currently contention for the video hardware is not considered an error, apparently new clients just get queued while waiting to use the hardware.  I suspect that changing that behavior at this point would be considered too risky.  And I'm not particularly optimistic that many apps or web pages will handle errors in any useful way

In any case, it is clear that videocontrols.xml is not a way forward for this bug.

I wonder if gecko could be modified so that the video element could display an icon when this happens, sort of the way that the <img> element can display a broken image icon when there is an error. That doesn't seem like something that could be fixed in v2.1, however.

Comment 70

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

There are two issues here:

1. There is only one decoder.
2. Gecko doesn't do a good job of managing decoder contention.

Neither of these are new and neither of them can be fixed quickly. Desktop will fall back to a software decoder and more recent android phones support multiple decoders so the problem is less pronounced.

Is #1 really a hardware limitation or can we support multiple decoders (perhaps with lower frame rates) somehow with clever drivers?

Comment 71

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #70)
> There are two issues here:
> 
> 1. There is only one decoder.
> 2. Gecko doesn't do a good job of managing decoder contention.


> Is #1 really a hardware limitation or can we support multiple decoders
> (perhaps with lower frame rates) somehow with clever drivers?

Basically: no without some tricks. It needs to have internal state/frame-buffers for reference frames/SPS/PPS settings/etc.  

The closest you could come would be hacky, but might work:
init decoder
decode one GOP sequence from stream A including SPS/PPS
re-init decoder
decode one GOP sequence from stream B including SPS/PPS
Lather, rinse, repeat.  So long as there are no LTRs or other funky stuff, this will likely mostly work.  Not sure it will be very efficient depending on decoder re-init time.   And you have to parse the stream to pick out the GOP (not that hard).

Note: this only applies to streaming decode multiplexing.  realtime (GOP-less) decode and encode won't work well with this.  (You might be able to play a similar trick with encode by forcing a GOP structure; GOP-less decode not so much.)

Comment 72

[Russ Nicoletti [:russn]]

(In reply to David Flanagan [:djf] from comment #61)
> Comment on attachment 8513693 [details] [review]
> Github PR: https://github.com/mozilla-b2g/gaia/pull/25627
> 
> Russ,
> 
> I left a few comments on github asking for minor code cleanup.
> 
> I think you may also need to add the same fix to view.js, don't you?
> 
> Also, please check what happens if you start a video in the video app, then
> go to the homescreen, and then start a webrtc call, and then return to the
> video app. I think that this patch will not correctly handle that case.
> 
> If not, then I think maybe you need to write a ensurePlaybackStarts()
> function. Call it when the video.src property is first set, and also call it
> every time that play() is called on the video. This function could probably
> listen for a 'playing' event. And, if you structure it with nested
> functions, I think that the timeout id could be a local variable instead of
> a global. If you put it in a new file, then you can use it for both the
> regular and the view activity case.

The patch does not handle the scenario you described. However, listening for a 'playing' event would not address that use case. On a 'visibilitychange' (when going to the background) the video app pauses the video (if it's playing). When coming to the foreground, the video will be paused; we will not get a 'playing' event. 

Thinking more about all the use cases where there can be contention for the hw video codec, I can think of four:

1) User opens video app and selects video to play from thumbnail list after starting WebRTC session.
2) After playing a video, users puts video app in background, starts WebRTC session, brings video app to foreground
3) User shares a video via an sms message (pick acvitity) after starting a WebRTC session.
4) User plays a video attachment to an email after starting a WebRTC session.

In scenarios 2 and 3, there will be no 'playing' event -- the video is not played in those scenarios -- but there will be 'loadedmetadata' events. In scenarios 1 and 4, there will be a 'playing' events as well as 'loadedmetadata' events.

So the common denominator in all these scenarios is the 'loadedmetadata' event. Therefore, in all these scenarios we need to call an ensureVideoLoads function, similar to the ensurePlaybackStarts you suggested, that listens for the 'loadedmetadata' event and if it doesn't receive it within a certain amount of time displays the ' video-hardware-in-use' message. This is what I'm in the process of implementing; it's almost finished.

Comment 73

[David Flanagan [:djf]]

See bug 1088456 for a example of why we should address this for the view activity as well.

Comment 74

[David Flanagan [:djf]]

(In reply to Russ Nicoletti [:russn] from comment #72)

> The patch does not handle the scenario you described. However, listening for
> a 'playing' event would not address that use case. On a 'visibilitychange'
> (when going to the background) the video app pauses the video (if it's
> playing). When coming to the foreground, the video will be paused; we will
> not get a 'playing' event. 
> 

Yeah. What I meant (but did not write explicitly) is that the user taps the play button after the video app comes to the foreground. Then there would be a playing event.  And I don't think we can count on getting a metadataloaded event in this case. Gecko maybe sending us one now, but if it is, I think that is a bug.


> 3) User shares a video via an sms message (pick acvitity) after starting a
> WebRTC session.

It looks like in this case we don't auto play the video, right? So unless the user tries to play it, we don't actually have contention for the hardware and we ought to allow them to click the Done button and complete the pick activity. If they do try to play the video, then I'd expect to see the overlay displaying an error message. But we want to be careful to ensure that the done and cancel buttons are still available so that the user doesn't get stuck in that scenario.

This is a more complicated workaround than it first appeared!

Comment 75

[David Flanagan [:djf]]

So the Hello app is pretty hard to figure out... I did finally get it installed. The key to making it work (at least after install with 'grunt build') seems to be to explicitly go into Settings > App Permissions and grant all the permissions that it needs. There must be a bug where the dialogs asking for camera permission is hidden behind other content.  But on both master and 2.1, I can get it to work if I do that, and I'm able to make a call to my firefox account and receive it in my browser.

And when I do that, I find that I can actually play a video while on a call.  It was a video recorded by the camera. It worked.  After doing that, I was never able to get back to the video for my Hello call. I could still hear it so I know I was still connected, but I never saw the call screen again. No way to hang up from my Flame, either.

I was calling between desktop and a Flame device. Maybe my desktop encodes the video in some format that is decoded in software on the Flame, so that this bug does not occur in that scenario. Maybe I have to actually call another Flame.

Comment 76

[David Flanagan [:djf]]

Also, I found that the Hello app experience is pretty crappy with 319mb, so I'm now running with the full 1024mb of memory. And stuff on master was pretty unstable, so I'm testing with today's 2.1 nightly build instead of using master.

Comment 77

[David Flanagan [:djf]]

I've made the edits described in comment #14 and comment #16 and can confirm that I see this line in my logcat: 

[main|WebrtcVideoSessionConduit] VideoConduit.cpp:552: ConfigureSendMediaCodec for H264_P1

So I think that means that I am using H264, right?

Comment 78

[David Flanagan [:djf]]

The video I've been playing during the calls is a .3gpp video from the camera, but I'm pretty sure that is H264 in an mp4 container and that it uses the hardware. So I'm not sure why I can play that and keep the loop connection going.

I have updated to base image v188 if that makes any difference.

Comment 79

[David Flanagan [:djf]]

Russ and I were finally able to reproduce this bug. If we both follow the steps in comment #14 and do a device to device call, we can reproduce the bug, and Russ can verify that his patch is being triggered.  If we do a device to desktop call, or a device to device call where one of us has not updated the prefs.js file as in comment #14 then the bug does not reproduce, presumably because we're not using h264.

As far as I can tell from the comments above, this is the first time that this bug has ever been reproduced at Mozilla...

Russ has a patch that is almost ready, and I think we can review it and land it tomorrow 10/31. Updating the whiteboard to reflect that.

Comment 80

[Russ Nicoletti [:russn]]

Comment on attachment 8513693 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25627

I have updated the PR to address David's review comments and to fix issues discovered during our testing.

Comment 81

[David Flanagan [:djf]]

Russ: I've left comments on github. Please make another pass on the PR and ping me for another review.

Comment 82

[Russ Nicoletti [:russn]]

(In reply to David Flanagan [:djf] from comment #81)
> Russ: I've left comments on github. Please make another pass on the PR and
> ping me for another review.

I need to do more testing before finalizing the PR. My belief is that we may have to have to use the 'loadedmetadata' event instead of the 'playing' event. From my testing this morning, it appears the hw codec contention happens when loading metadata (it seems we don't get the 'loadedmetadata' event when loading a video while there is a webRTC call in progress). If this is true, the theory that, for example, pick activities will be functional while webRTC is in progress is false and in fact will produce poor user experience since we won't show a message if we're only focusing on when the video is played.

I will follow up after completing the testing later this afternoon.

Comment 83

[David Flanagan [:djf]]

That surprises me. I thought that the patch you had didn't set the 'playing' event handler until after it got the 'metadataloaded' callback. So if it was working at all in the non-pick case, you must have been getting the metadataloaded and then not getting the playing event.

If you do switch to monitoring metadataloaded, though, I still suspect you might need to monitor the playing callback in the case where you start playing a video, pause it, start a Hello call, return the the video app and resume playing. In that case if we get a metadataloaded event it is probably a bug, and I'd rather not depend on it.

Also, if you don't think this is going to be ready to land today please update the ETA in the whiteboard, for the benefit of the release managers who are tracking this bug.

Comment 84

[Russ Nicoletti [:russn]]

Comment on attachment 8513693 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25627

As discussed with David over IRC, I have removed the part of the patch that checks for a loadedmetadata event when the video app is coming into the foreground. This is due to the concern that as gecko supports multiple hardware decoder units, the pause-homescreen-video-play scenario will become the same as pause-play scenario where there is no reason to expect a loadedmetadata event.

Therefore, the decision to not address the 'video app play video-go to homescreen-initiate Hello app webRTC call-go to homescreen-resume video' scenario is that it's not worth it to address this rare use case by introducing code that can break the more common 'video app play video--go to homescreen-come back to video app and resume video' scenario.

The PR has been updated accordingly and is ready for review.

Comment 85

[David Flanagan [:djf]]

Comment on attachment 8513693 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25627

r+ if you fix index.html, video.css, and view.html to change the element id 'overlay-content' to 'in-use-overlay-content'. As the patch stands now you've got two elements with the same id, which is invalid HTML, I think.

As Russ describes above, this patch does not fix the case where the user starts playing a video, pauses it, starts a Hello call, then tries to resume the video. In that case, the video will not resume and no message will be displayed. Russ had a patch that would fix that, but I made him take it out because in my judgement the risk of serious future bugs from that patch is worse than this existing bug. If it is absolutely necessary to address this 'resume video already in progress' issue, please file a followup bug and assign it to me.

Meanwhile, I'll file followup bugs to see if we can get a better solution in gecko for 2.2

Comment 86

[David Flanagan [:djf]]

I've filed bug 1093283 to try to get a better fix into gecko. If anyone here knows who can drive that bug, please alert them to it.

Comment 87

[Inder]

Jay -- can you please provide feedback on the fix and the case it doesn't fix.

Comment 88

[Russ Nicoletti [:russn]]

Attachment: Github PR for 2.1 branch: https://github.com/mozilla-b2g/gaia/pull/25783

David, I changed the 'overlay-content' element that you mentioned in your PR comments. I've made that change to this 2.1 PR as well. The only difference in this PR compared to master is the 'video-hardware-in-use-title' is set to 'error-unknown' so that there are no string changes. I'm not using 'video-hardware-in-use-text' because, given we cannot create any new strings, I don't know what to use for that; using just the title looks fine to me. I will attach a screenshot of the error dialog for you to review.

Comment 89

[Russ Nicoletti [:russn]]

Attachment: 2.1 'video in use' error dialog

Comment 90

[David Flanagan [:djf]]

(In reply to Russ Nicoletti [:russn] from comment #88)
> Created attachment 8516293 [details] [review]
> Github PR for 2.1 branch: https://github.com/mozilla-b2g/gaia/pull/25783
> 
> David, I changed the 'overlay-content' element that you mentioned in your PR
> comments. I've made that change to this 2.1 PR as well. The only difference
> in this PR compared to master is the 'video-hardware-in-use-title' is set to
> 'error-unknown' so that there are no string changes. 

Actually, the decision about the strings was that we would use the new strings in 2.1, but they would not be localized, so by default users will see English in all locales. The hope is that once the new strings are localized in 2.2 our partners can cherry-pick those strings into their 2.1 trees. This is what was done in the camera bug, at least, so I think the 2.1 patch should just be a straight uplift of the patch for master.

Comment 91

[David Flanagan [:djf]]

This patch has r+ and is ready to land. But the Gaia tree is closed, so it has not landed yet. We will request 2.1 uplift approval when it lands.

Comment 92

[David Flanagan [:djf]]

Comment on attachment 8516293 [details] [review]
Github PR for 2.1 branch: https://github.com/mozilla-b2g/gaia/pull/25783

r- for the reason given in comment #90. I think we just want a straight uplift and will use the strings untranslated. I think that Hema and Stephany signed off on that approach

Comment 93

[Russ Nicoletti [:russn]]

Ok, I will take no action regarding the 2.1 PR. I would close it but it seems I don't have the privileges.

Comment 94

[Russ Nicoletti [:russn]]

Master:

https://github.com/mozilla-b2g/gaia/commit/7bac3bc2379453196a95117e72670009a4d856e0

Comment 95

[Russ Nicoletti [:russn]]

Comment on attachment 8513693 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25627

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 
Issue caused by Hello app (webRTC) holding on to the video codec, not allowing video app access to it

[User impact] if declined:
Bad user experience when user attempts to play a video while having a webRTC session -- video app will not display any message, just shows spinner forever.

[Testing completed]:
Manual testing with Hello app and unit tests

[Risk to taking this patch] (and alternatives if risky):
Video app may temporarily display "Cannot load video" if the loadedmetadataevent from gecko comes later than it should (more than 500ms) when loading a video. Other alternatives -- gecko changes -- are much more complicated and risky. Also, see this comment for one use-case that is not addressed by this change: https://bugzilla.mozilla.org/show_bug.cgi?id=1079519#c85

[String changes made]:
Introduction of new error dialog title and text. However, the decision (from Hema and Stephany) about the strings was that we would use the new strings in 2.1, but they would not be localized, so by default users will see English in all locales. The hope is that once the new strings are localized in 2.2 our partners can cherry-pick those strings into their 2.1 trees.

Comment 96

[Jay]

(In reply to Inder from comment #87)
> Jay -- can you please provide feedback on the fix and the case it doesn't
> fix.

The short term fix looks fine as long as it doesn't cause any freeze.

Comment 97

[[:fabrice] Fabrice Desré]

Comment on attachment 8513693 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25627

We can't take new strings anymore like the ones added at https://github.com/mozilla-b2g/gaia/pull/25627/files#diff-55698edbdb20279da9494aafdf4c3d48R51

Comment 98

[Russ Nicoletti [:russn]]

Comment on attachment 8513693 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25627

Fabrice, I realize it's too late for new strings. However, we wanted to handle this bug like bug 1079543. Bhavana approved that for 2.1 uplift. Both bugs are the same issue (video cannot be played when a webRTC session is in progress). Video app and camera app have the same fix: to display a message to the user -- it's the quickest, least risky solution given the timeframe.

Since Bhavana approved 1079543, requesting her approval for this bug.

Comment 99

[[:fabrice] Fabrice Desré]

Hm, I thought we were shipping with hardcoded strings in 2.1 for these, which is different.
It's not about risk per se, but about issues for the localizers, QA, etc.

Comment 100

[David Flanagan [:djf]]

I just noticed that for the view activity the new overlay introduced by this bug ends up on top of the back button. So the user sees a message but ends up trapped, which is probably worse than the original view activity situation where the user just saw a spinner but could press a back button to get out.

Since we haven't uplifted this yet and are still figuring out the strings, I'm going to re-open this and take it to fix.

Comment 101

[David Flanagan [:djf]]

Attachment: PR to revert the patch from master

Comment 102

[David Flanagan [:djf]]

I've confirmed that the patch we landed obscures the back button in the view activity, so we've got to revert this.  The gaia tree is closed at the moment, so I've attached a PR to do the revert.

Comment 103

[David Flanagan [:djf]]

Reverted the patch on master: https://github.com/mozilla-b2g/gaia/commit/36f0fa13181046bbf973c1defda60ca0aac47324

I should have a revised PR ready for review shortly.

Comment 104

[David Flanagan [:djf]]

Attachment: updated PR

Russ: 

This PR has two commits (which I'll squash after review). The first is your original. The second are my changes, which ensure that the back button is visible when the error is shown in the view activity and add code to handle the resume playing case.

We'll still need to test this on an actual Hello call when we're both online at the same time.  

(And it occurs to me now that we may have the same issue of needing to show the back button during the pick activity. So I may need to update the PR for that.)

Comment 105

[David Flanagan [:djf]]

Updated the PR to not hide the back button for the pick activity either.

Note that now that the patch has code to handle the case when we try to resume playing a paused video, that is another case where the error overlay hides the back button that would otherwise let the user go back to the list of videos. In this case, I do obscure that button intentionally because actually letting the user push that back button would cause other problems (like not being able to capture a thumbnail of the current frame). So in this case, the user is trapped (just as they would be if when initially clicking on a thumbnail) and all they can do is press the home button to return to the homescreen or pull down the notification tray to return to the webrtc call.

Comment 106

[David Flanagan [:djf]]

Updated the PR one more time. It is ready for review now.

Comment 107

[Russ Nicoletti [:russn]]

Comment on attachment 8518528 [details] [review]
updated PR

David, I've updated the PR with my comments and questions. I'm giving r+, based on my testing of the original patch and that your changes look good, and that I assume you've fully tested your changes. But I would like to see your comments on my questions before you land. Thanks.

Comment 108

[David Flanagan [:djf]]

Review comments addressed and commits squashed. Just waiting for tests to run now.

Comment 109

[David Flanagan [:djf]]

Comment on attachment 8518528 [details] [review]
updated PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Hello app caused this regression

[User impact] if declined: no explanation for videos that don't play when a Hello call is in the background.

[Testing completed]: it is very hard to test this because it is hard to establish a Hello call between two flame devices, and manually update the user.js prefs file on both devices to force them to use h264. But yes, Russ and I were able to do that, reproduce the bug, and test this fix.

[Risk to taking this patch] (and alternatives if risky): It is not a trivial patch. Not horribly risky, though. The worst thing that could go wrong is that we could mistakenly think that the video hardware is in use when it isn't actually and could display the "can't play video" error message to the user when the video could actually play. I don't think that will happen. It is possible that when the phone is under very heavy load and swapping, we may temporarily show the error message because the video does not respond promptly. This will correct on its own however, and when the video begins playing the error message will go away.

[String changes made]: Yes, we added two new strings, and per the long email thread about this and the similar Camera bug, the feeling is that an untranslated error message is better than no error message at all, so the plan is to uplift this even though the late-l10n deadline is past. Bug 1095673 and bug 1095686 may help to mitigate this.

Comment 110

[David Flanagan [:djf]]

Bug 1093283 has been filed to get a better solution for this bug into gecko. When that lands, we'll want to revert this patch from master and use the new gecko feature to detect the hardware-in-use condition.

Comment 111

[David Flanagan [:djf]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/256dc078d6d7e8669a2eab3e89dd09c526fef608

Comment 112

[Tony Chung [:tchung]]

Lets get this tested on master first, and also after its been uplifted to 2.1

Comment 113

[bhavana bajaj [:bajaj]]

(In reply to Tony Chung [:tchung] from comment #112)
> Lets get this tested on master first, and also after its been uplifted to 2.1

Joshua, can QA please help with this urgently?

Comment 114

[Joshua Mitchell (Inactive)]

based on comment 3, comment 7, comment 15 and a few other's; nobody here was ever able to repro this bug. Based on that it would not be appropriate for us to test the latest to verify it as fixed because if we can not reproduce, then not-seeing it in the newest build is not a valid test of it being 'fixed' only more no-reproducing. I suggest re-directing to the reporter, assignee or other reproducing party for verification.

Comment 115

[bhavana bajaj [:bajaj]]

(In reply to Joshua Mitchell [:Joshua_M] from comment #114)
> based on comment 3, comment 7, comment 15 and a few other's; nobody here was
> ever able to repro this bug. Based on that it would not be appropriate for
> us to test the latest to verify it as fixed because if we can not reproduce,
> then not-seeing it in the newest build is not a valid test of it being
> 'fixed' only more no-reproducing. I suggest re-directing to the reporter,
> assignee or other reproducing party for verification.

Thanks joshua ! Jay NI you to confirm the fix and getting this uplifted in parallel.

Comment 116

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/1654582916815399578d5d41c203bb44d10a2370

Comment 117

[Jay]

(In reply to bhavana bajaj [:bajaj] from comment #115)
> (In reply to Joshua Mitchell [:Joshua_M] from comment #114)
> > based on comment 3, comment 7, comment 15 and a few other's; nobody here was
> > ever able to repro this bug. Based on that it would not be appropriate for
> > us to test the latest to verify it as fixed because if we can not reproduce,
> > then not-seeing it in the newest build is not a valid test of it being
> > 'fixed' only more no-reproducing. I suggest re-directing to the reporter,
> > assignee or other reproducing party for verification.
> 
> Thanks joshua ! Jay NI you to confirm the fix and getting this uplifted in
> parallel.

I saw the error message with previous patch, but I don't see the error message with the latest patch. However, at least, with the latest patch, it doesn't freeze the video app like before.

Comment 118

[Yeojin Chung [:YeojinC]]

Unable to verify this bug as we were unable to reproduce the original issue. See comment 114.