1434439 - Firefox can get stuck in mode where it always downscales camera video unnecessarily.

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

+++ This bug was initially created as a clone of Bug #1434353 +++

This bug is about the unnecessary downscaling, not the noise in Bug 1434353.

We don't have good STRs yet on how to reproduce this, but I have Nightly in this state at the moment. See Bug 1434353 comment 2 for how to detect it using profiler.

Without reliable STRs, I don't have a regression window, so marking it a regression from bug 1388219 where camera downscaling was introduced.

This reduces performance during WebRTC calls unnecessarily, so we need to find it.

Comment 1

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

I know a case which can cause the unnecessary downscaling.
Not sure if this was what you encountered.
STRs:

1. First, open a page requesting 640x480
2. In a second tab (running in different content process), open a page requesting 720p (Now downscaling is enabled as expected)
3. Turn off the second tab.

In the current design, we will still set the camera to 720p and do down-scaling for the first tab.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 59 to make sure we follow up. We could still take a patch in 59 beta.

Comment 3

[Julien Cristau [:jcristau] (back April 22)]

Are we any closer to STR and a fix here?  59 is approaching fast :/

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

There's a chance this is bug 1441409. Will investigate more tomorrow.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Bug 1441409 looks like the culprit.

That said, I found through code inspection another potential case of this where we forget to clean up a capability requirement, which in theory could cause Firefox to get stuck in downscaling mode.

It's not the case I saw in comment 0, as this particular failure cannot happen on OSX, but it may be an issue on other platforms.

The symptoms of this would manifest as "InternalError: Starting video failed", which is a later failure point than the more common "NotReadableError". Causing a late-failure like this might require timing, or maybe a problem resolution like 736x736 on platforms like Android where we've seen certain capabilities not work.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1434439 - Avoid stray capability requirement in StartCapture failure case.

Review commit: https://reviewboard.mozilla.org/r/223590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223590/

Comment 7

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8954551 [details]
Bug 1434439 - Avoid stray capability requirement in StartCapture failure case.

https://reviewboard.mozilla.org/r/223590/#review230178

::: dom/media/systemservices/CamerasParent.cpp:950
(Diff revision 1)
>            if (!error) {
>              cap.VideoCapture()->RegisterCaptureDataCallback(
>                static_cast<rtc::VideoSinkInterface<webrtc::VideoFrame>*>(*cbh));
> +          } else {
> +            sDeviceUniqueIDs.erase(capnum);
> +            sAllRequestedCapabilities.erase(capnum);
>            }

Make the failure case a guard with early return:

```
if (error) {
  sDeviceUniqueIDs.erase(capnum);
  sAllRequestedCapabilities.erase(capnum);
  return;
}
```

I know we want to keep things small for minimal uplifts, but this is still trivial IMHO.

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8954551 [details]
Bug 1434439 - Avoid stray capability requirement in StartCapture failure case.

https://reviewboard.mozilla.org/r/223590/#review230180

::: dom/media/systemservices/CamerasParent.cpp:954
(Diff revision 1)
> +            sDeviceUniqueIDs.erase(capnum);
> +            sAllRequestedCapabilities.erase(capnum);

Hmm, are we sure these were the first to be added for capnum?

The emplace above is defensive and that is sometimes good, but here it could be masking an existing entry. In that case we'd rather like to replace the entry with the old pre-existing capability.

I'm ok with keeping the `erase` if we add diagnostic asserts for this.

There's already one debug-only for the device ID. Please make it diagnostic and add another for the capabilities map.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

We are building the release candidate today, so this has just missed the cutoff.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8954551 [details]
Bug 1434439 - Avoid stray capability requirement in StartCapture failure case.

https://reviewboard.mozilla.org/r/223590/#review230178

> Make the failure case a guard with early return:
> 
> ```
> if (error) {
>   sDeviceUniqueIDs.erase(capnum);
>   sAllRequestedCapabilities.erase(capnum);
>   return;
> }
> ```
> 
> I know we want to keep things small for minimal uplifts, but this is still trivial IMHO.

`error` is passed back in `ipc_runnable` below. An early return would make that not happen.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8954551 [details]
Bug 1434439 - Avoid stray capability requirement in StartCapture failure case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223590/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c3ae4a1a7ed
Avoid stray capability requirement in StartCapture failure case. r=pehrsons

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3c3ae4a1a7ed