Closed Bug 970725 Opened 9 years ago Closed 5 years ago

Adapt gUM capture resolution based on encoding resolution

Categories

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

defect

Tracking

()

RESOLVED INACTIVE
tracking-b2g backlog

People

(Reporter: abr, Assigned: ctai)

References

Details

(Whiteboard: [WebRTC][ft:multimedia-platform])

Attachments

(1 file, 4 obsolete files)

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
Blocks: 970426
Depends on: 877954
This should not block a user-facing application; it would be nice on low-end mobile devices especially, but not mandatory especially on desktop.
No longer blocks: 970426
Would it be possible to support constraints in gUM? e.g.

  var constraints = { constraints:
                      { video: {
                          mandatory: {
                            minWidth: 1280,
                            maxWidth: 2304
                          }
                        },
                        audio: true
                      }
                    };
(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.
Assignee: nobody → slee
>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.
(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.
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.
Attached patch patch (obsolete) — Splinter Review
This patch is to measure the time of restarting camera.
Attached patch patch (obsolete) — Splinter Review
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
Attachment #8397595 - Attachment is obsolete: true
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.
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.
(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.
Flags: needinfo?(mhabicher)
(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
Flags: needinfo?(mhabicher)
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.
After talk with slee, I will take this bug.
Assignee: slee → ctai
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.
No longer blocks: 984239
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).
Depends on: 986513
blocking-b2g: --- → backlog
Whiteboard: [WebRTC][ft:multimedia-platform]
Status: NEW → ASSIGNED
Attached patch bug-970725.patch-v1 (obsolete) — Splinter Review
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.
Attachment #8397599 - Attachment is obsolete: true
Attachment #8417786 - Flags: feedback?(slee)
Attached patch bug-970725.patch-v1.1 (obsolete) — Splinter Review
Fixed build-break in desktop build.
Attachment #8417786 - Attachment is obsolete: true
Attachment #8417786 - Flags: feedback?(slee)
Attachment #8417941 - Flags: feedback?(slee)
Attachment #8417941 - Flags: feedback?(slee) → feedback+
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.
Modified according to slee's comments.
Attachment #8417941 - Attachment is obsolete: true
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.
Attachment #8422327 - Flags: feedback?(rjesup)
blocking-b2g: backlog → ---
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).
backlog: --- → webRTC+
Rank: 38
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
Priority: -- → P3
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
Flags: needinfo?(jib)
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
Closing because of inactivity.

Feel free to re-open if you think this is still an issue.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(rjesup)
Resolution: --- → INACTIVE
Attachment #8422327 - Flags: feedback?(rjesup)
You need to log in before you can comment on or make changes to this bug.