970725 - Adapt gUM capture resolution based on encoding resolution

Status: RESOLVED INACTIVE

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

Description

[Adam Roach [:abr]]

When the encoding resolution changes (e.g., based on reported bandwidth), we need to adjust the camera capture resolution to match the encoding resolution as closely as possible

Comment 1

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

This should not block a user-facing application; it would be nice on low-end mobile devices especially, but not mandatory especially on desktop.

Comment 2

[Silvia Pfeiffer]

Would it be possible to support constraints in gUM? e.g.

  var constraints = { constraints:
                      { video: {
                          mandatory: {
                            minWidth: 1280,
                            maxWidth: 2304
                          }
                        },
                        audio: true
                      }
                    };

Comment 3

[StevenLee[:slee]]

(In reply to Silvia Pfeiffer from comment #2)
> Would it be possible to support constraints in gUM? e.g.
I think what abr wants is to change the camera capture resolution by current environment(CPU loading, network traffice, ...). MediaConstraints just limits the maximum resolution in the beginning. Ex, if network is busy but the cpu is affordable for the current setting, we could ask camera to output smaller video frames to get smooth call.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

>I think what abr wants is to change the camera capture resolution by current environment(CPU loading, network traffice, ...).

No, by the current encoding resolution. See the original description. The encoding resolution will be adapted to the environment in bug 877954, but there is no need to capture in higher resolution than we encode.

Comment 5

[StevenLee[:slee]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #4)
> No, by the current encoding resolution. See the original description. The
> encoding resolution will be adapted to the environment in bug 877954, but
> there is no need to capture in higher resolution than we encode.
Thanks for the clarification. My description may mislead.

Comment 6

[StevenLee[:slee]]

I did a simple test on flame to measure the needed time of re-start camera with different resolution. It shows that the total time is about 1.5x seconds. I tried just change the preview size but it does not work.

Comment 7

[StevenLee[:slee]]

Attachment: patch

This patch is to measure the time of restarting camera.

Comment 8

[StevenLee[:slee]]

Attachment: patch

remove some unused code. BTW, one thing to note, when restarting the camera, there's no permission dialog prompt. So I need to modify GonkPermissionService to always grant the permission

Comment 9

[StevenLee[:slee]]

One more thing, when we change the input size(resolution or rotation) dynamically, HW codec may have problems. After discussing with jhlin who is working on bug 911046, he said that HW codec is not able to handle the situation. It needs to re-initialize the codec. We may need to test it when HW codec is landed.

Comment 10

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

The hardware codec code *must* handle resolution changes (H.264 supports it).  If need be, it can handle a resolution change by stopping and restarting the hardware encode (new IDR, etc); but it must handle it.

Comment 11

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

(In reply to StevenLee[:slee] from comment #6)
> I did a simple test on flame to measure the needed time of re-start camera
> with different resolution. It shows that the total time is about 1.5x
> seconds. I tried just change the preview size but it does not work.

Ouch.  MikeH, any idea why this takes so long?  If so, we'll' have to avoid changing the camera res in mid-call and just use SW scaling (which adds more CPU/GPU time).  This also makes GPU scaling/rotation more useful/important.

Comment 12

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

(In reply to Randell Jesup [:jesup] from comment #11)
>
> (In reply to StevenLee[:slee] from comment #6)
>
> > I did a simple test on flame to measure the needed time of re-start camera
> > with different resolution. It shows that the total time is about 1.5x
> > seconds. I tried just change the preview size but it does not work.
> 
> Ouch.  MikeH, any idea why this takes so long?  If so, we'll' have to avoid
> changing the camera res in mid-call and just use SW scaling (which adds more
> CPU/GPU time).  This also makes GPU scaling/rotation more useful/important.

Unfortunately I don't have a flame to test this against, but it's possible. IIRC, Hamachi takes ~2 seconds to initialize it's camera, and all of that time is buried in the driver initialization code, into which we unfortunately don't have any visibility.

That said, the Gonk camera API supports resolution changing on the fly. You just need to call setConfiguration()[1] with the new desired settings. That call will handle the sequencing requirements behind the scene and should happen quickly.

1. http://dxr.mozilla.org/mozilla-central/source/dom/camera/ICameraControl.h#127

Comment 13

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

If you're not talking about b2g, the equivalent on Android is:
- stopPreview()
- CameraParameters::SetPreviewSize( <new size> )
- startPreview()

Again, this should happen fairly quickly. If it doesn't please file a separate bug againt Flame, as it's going to be an issue on b2g as well.

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

After talk with slee, I will take this bug.

Comment 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

The encoding resolution is re-calculated in |VCMQmResolution::UpdateCodecResolution|. Get to use the new resolution to change the camera preview resolution via |setConfiguration| in camera control.

Comment 16

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

Bug 970725 definitely does not block landing a working H.264 implementation; the webrtc.org code will rescale camera video as needed in the CPU before encoding.  This is an (important) CPU-use optimization, especially when the not-yet-landed code for changing encode resolution in response to CPU load and bandwidth restrictions lands (bug 986513).

Comment 17

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-970725.patch-v1

This patch do following works:
1. Add |SetPreviewResolution| into MediaEngineSource related code.
2. Extend ViEEncoderObserver by adding |VideoQMSettingsChanged|.
3. Add |GetWindowID| into PeerConnectionMedia.
4. Call |CreateAndRegisterEncoderObserver| in vcmTxStartICE_m to bind the VideoEncoderObeserver.
5. Now once |QMVideoSettingsCallback::SetVideoQMSettings| is called, ViEEncoder will trigger the ViEEncoderObserver to notify the VideoQMSettings changes. Then VideoConduit will change camera preview resolution through media manager.

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-970725.patch-v1.1

Fixed build-break in desktop build.

Comment 19

[StevenLee[:slee]]

Comment on attachment 8417941 [details] [diff] [review]
bug-970725.patch-v1.1

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

::: content/media/webrtc/MediaEngine.h
@@ +115,5 @@
>  
> +  /* Set preview resolution to the closet supported size. */
> +  virtual nsresult SetPreviewResolution(const uint32_t width,
> +                                        const uint32_t height) = 0;
> +

I think you could implement a default version here then you don't have to implement the dummy functions in other classes.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +52,5 @@
> +  virtual void VideoQMSettingsChanged(const uint32_t width,
> +                                      const uint32_t height) {
> +#ifdef MOZILLA_INTERNAL_API
> +    MediaManager* mediaManager = MediaManager::Get();
> +    StreamListeners *listeners = mediaManager->GetWindowListeners(mWindowID);

This function call be called only on main thread. You may dispatch a runnable to main here then you don't need to dispatch in MediaEngineWebRTCVideoSource::SetPreviewResolution.

Comment 20

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-970725.patch-v1.2

Modified according to slee's comments.

Comment 21

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8422327 [details] [diff] [review]
bug-970725.patch-v1.2

:jesup, Could you please give me a feedback for this patch?

This patch do following works:
1. Add |SetPreviewResolution| into MediaEngineSource related code.
2. Extend ViEEncoderObserver by adding |VideoQMSettingsChanged|.
3. Add |GetWindowID| into PeerConnectionMedia.
4. Call |CreateAndRegisterEncoderObserver| in vcmTxStartICE_m to bind the VideoEncoderObeserver.
5. Now once |QMVideoSettingsCallback::SetVideoQMSettings| is called, ViEEncoder will trigger the ViEEncoderObserver to notify the VideoQMSettings changes. Then VideoConduit will change camera preview resolution through media manager.

Comment 22

[Maire Reavy [:mreavy]]

Poke - though I think jesup will comment that feeding resolution changes back to GUM is more complex than this, and may also involve spec work.  There are issues when the camera stream is also being used for self-image, for example, though usually those are small.  But doing it in some other cases (local recording of captured video while in a call) would be affected.  This may need to be opt-in by the application (and spec work required for that).

Comment 23

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

I think we're fine (within the limits of application-provided constraints).

The Media Capture spec [1] says "this is an optional optimization that the User Agent is allowed to make".

Specifically about PeerConnection:

"Note that this specification does not define a mechanism by which a change to the remote client's video source could automatically trigger a change to the home client's video source. Implementations may choose to make such source-to-sink optimizations as long as they only do so within the constraints established by the application".

The spec examples suggest that UAs should not lower the capture resolution below a self-image.

If local recording is happening, this would necessarily prevent optimization, is how I interpret things.

Provided we respect all these things, we should not need opt-in I hope.

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#the-model-sources-sinks-constraints-and-settings

Comment 24

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 25

[Nils Ohlmeier [:drno]]

Closing because of inactivity.

Feel free to re-open if you think this is still an issue.