912342 - Change camera resolution with MediaStreamTrack.applyConstraints()

Status: RESOLVED FIXED

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

Description

[Shian-Yow Wu [:swu]]

Filed for bug 881935 comment 46.

In case we need to rescale sending video to lower resolution, we may also want to reduce camera capture resolution in runtime, to save local CPU resource on low end devices.

Does it make sence?

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Change camera capture capability

Tim,

When video resolution was changed in WebrtcVideoConduit::SelectSendResolution(), is there a 
simple way to change camera capability in MediaEngineWebRTCVideoSource?

It appears the way in the WIP patch doesn't work, mPtrViECapture is not the correct pointer we want.

Comment 2

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

That patch changes the capture resolution (on the wrong channel) to match what the resolution of the already-being-captured frames are, which even if done in MediaEngineWebRTCVideoSource would be an effective no-op.

The comment you link to is more correct (though you also will want to link to the CPU load detection that :gcp is working on, and for that do it during a capture).  

You can globally adjust the capture resolution using preference vars (see MediaManager)

Comment 3

[Shian-Yow Wu [:swu]]

(In reply to Randell Jesup [:jesup] from comment #2)
> That patch changes the capture resolution (on the wrong channel) to match
> what the resolution of the already-being-captured frames are, which even if
> done in MediaEngineWebRTCVideoSource would be an effective no-op.
> 
> The comment you link to is more correct (though you also will want to link
> to the CPU load detection that :gcp is working on, and for that do it during
> a capture).  
> 
> You can globally adjust the capture resolution using preference vars (see
> MediaManager)

Got it, thank you.

Comment 4

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Change camera capture capability

Comment 5

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Patch: Change camera capture capability

Randell,

This is a WIP patch working for desktop.
May I have your feedback?

Comment 6

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

Comment on attachment 820291 [details] [diff] [review]
(WIP) Patch: Change camera capture capability

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

r- but overall you're heading down a reasonable path, so f+.  The big issues are the enqueue-of-N-restarts and avoid restarting when it's not needed.

The config vars you added appear to be dups of the existing ones.  Either just use the existing ones, or rename them.  I'm tempted to say rename them (and use capture.)

Note that we'll likely want something that will let us adjust these values on-the-fly via constraints/settings per the media capture specs.

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +273,5 @@
>  
>  nsresult
> +MediaEngineTabVideoSource::Restart(const mozilla::MediaEnginePrefs&)
> +{
> +  return NS_OK;

This is where you'd adjust the resolution of the tab capture I assume

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +403,5 @@
> +#ifdef MOZ_B2G_CAMERA
> +  // TODO
> +#else
> +  if (mState == kStarted && mInitDone) {
> +    ChooseCapability(aPrefs);

verify that the capability chose is different than the current one before stopping/restarting

::: dom/media/MediaManager.cpp
@@ +1104,5 @@
>        prefs->AddObserver("media.navigator.video.default_height", sSingleton, false);
>        prefs->AddObserver("media.navigator.video.default_fps", sSingleton, false);
>        prefs->AddObserver("media.navigator.video.default_minfps", sSingleton, false);
> +      prefs->AddObserver("media.navigator.video.capture_width", sSingleton, false);
> +      prefs->AddObserver("media.navigator.video.capture_height", sSingleton, false);

How are these different than media.navigator.video.default_width/height?

@@ +1473,5 @@
> +
> +  bool capture_config = false;
> +
> +  aBranch->GetBoolPref("media.navigator.video.capture_config_enabled",
> +                       &capture_config);

Why do we need a separate boolean to enable these?

@@ +1776,5 @@
> +        listeners->ElementAt(i);
> +        if (listener->Stream()) { // aka HasBeenActivate()ed
> +          // TODO: Make sure the correct listener was chosen if we have
> +          // multiple cameras(e.g. front/back).
> +          listener->RestartVideoCapture(aPrefs);

This is going to enqueue N separate restarts to the MediaThread.  a) what happens when one has run and the others haven't?  b) what happens when the second occurs - haven't we already reconfigured the capture for that set of shared streams?

Now, there may be more than one physical camera being captured, and we do need to reconfigure each one (each MediaEngineWebRTCVideo object).

Comment 7

[Shian-Yow Wu [:swu]]

Comment on attachment 820291 [details] [diff] [review]
(WIP) Patch: Change camera capture capability

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

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +273,5 @@
>  
>  nsresult
> +MediaEngineTabVideoSource::Restart(const mozilla::MediaEnginePrefs&)
> +{
> +  return NS_OK;

Thank you for point this out.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +403,5 @@
> +#ifdef MOZ_B2G_CAMERA
> +  // TODO
> +#else
> +  if (mState == kStarted && mInitDone) {
> +    ChooseCapability(aPrefs);

Yes, will do.

::: dom/media/MediaManager.cpp
@@ +1104,5 @@
>        prefs->AddObserver("media.navigator.video.default_height", sSingleton, false);
>        prefs->AddObserver("media.navigator.video.default_fps", sSingleton, false);
>        prefs->AddObserver("media.navigator.video.default_minfps", sSingleton, false);
> +      prefs->AddObserver("media.navigator.video.capture_width", sSingleton, false);
> +      prefs->AddObserver("media.navigator.video.capture_height", sSingleton, false);

The capture_width and capture_height are runtime values, they may be dynamically changed according to remote constrains (max-fs) or local constrains (local CPU load).

Although we have MediaEngine::DEFAULT_VIDEO_WIDTH and MediaEngine::DEFAULT_VIDEO_HEIGHT, the default resolution might be different on difference devices(desktop, mobile, TV, etc.), so keeping default resolution setting in preferences should be better.  How do you think?

@@ +1473,5 @@
> +
> +  bool capture_config = false;
> +
> +  aBranch->GetBoolPref("media.navigator.video.capture_config_enabled",
> +                       &capture_config);

If local CPU bandwidth is ok, we might not want reduce capture resolution, because:
1. Reducing capture resolution also reduced local preview resolution.
2. When there is multiple sharing on same camera capture, not every remote endpoint needs reduced resolution.

If the constraint comes from remote max-fs, we have bug 881935 to limit the sending resolution.

@@ +1776,5 @@
> +        listeners->ElementAt(i);
> +        if (listener->Stream()) { // aka HasBeenActivate()ed
> +          // TODO: Make sure the correct listener was chosen if we have
> +          // multiple cameras(e.g. front/back).
> +          listener->RestartVideoCapture(aPrefs);

If we have only one physical camrea, I think we can restart when the first found one.  Is it correct?

For more than one physical camera, now I am thinking that we may need different MediaEnginePrefs for each of them.

Comment 8

[Shian-Yow Wu [:swu]]

(In reply to Randell Jesup [:jesup] from comment #6)
> Note that we'll likely want something that will let us adjust these values
> on-the-fly via constraints/settings per the media capture specs.

Yes, it's good to have a simple way to modify these values on the fly.
Do you mean providing a common API to change these values, instead of changing these values from each module?

Comment 9

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Patch: Change camera capture capability

Changes in this patch:
1. Support restart on B2G
2. Only restart when capability changed

Comment 10

[Shian-Yow Wu [:swu]]

Unassign myself as I am not actively working on this for the time being.

Comment 11

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

I was about to file a bug to "Change camera resolution with MediaStreamTrack.applyConstraints()". Does it make sense to reuse this bug (and maybe build on this patch)?

Comment 12

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

Yes, probably in that it has useful mechanisms.

Note that this patch "swallow's it's own tail"; it changes capture resolution when it sees the capture resolution change....  Really it should be hooked to the Load Adaptation code (to cut capture resolution when we reduce encode resolution due to over-bandwidth or overload) - but that would require the load code to deal with the input resolution change and track the original and rescaled sizes instead of using 3/4 and 1/2 scalings of the input.

Comment 13

[wenjie.zhang]

Is there a plan to fix the issue? It is important to change video stream resolution on the fly.

Comment 14

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

I plan on doing this next once I'm done with Bug 1037389.

Comment 15

[wenjie.zhang]

Can you estimate when it will be landed on Firefox. Lot's of customers require the feature to change video characters including resolution, bitrate, fps change on the fly with the same ssrc on the peer browser, which no any change is needed in this feature support.

Comment 16

[wenjie.zhang]

BTW, do you know there is other way to change video stream character on the fly within the current released firefox version? Our customers need this feature urgently.

Comment 17

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

Bugzilla is for technical discussion, not scheduling estimates, but I hope soon (41).

Note that constraints control settings of the *camera*, not the video (and not all cameras support all resolutions + frame rates), whereas transcoding and bitrate controls will likely go into the new RTCRtpSender object, which is still being spec'ed.

All we have there right now is RTCRtpSender.replaceTrack() which lets you replace a track with a different one, without the receiver noticing (same ssrc) http://jsfiddle.net/8nem95k7

Comment 18

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

Attachment: MozReview Request: Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Comment 19

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

Attachment: MozReview Request: Bug 912342 - Move code MediaOperationTask from .h to .cpp

Bug 912342 - Move code MediaOperationTask from .h to .cpp

Comment 20

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

Attachment: MozReview Request: Bug 912342 - Change capture resolution

Bug 912342 - Change capture resolution

Comment 21

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

Work in progress, but works: https://jsfiddle.net/ftdmdkjt/

However, after playing with applyConstraints repeatedly, it inevitably runs into an assert:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient.cpp?rev=fc15644207d8&mark=164-164#127

    MOZ_ASSERT(texture == mBuffers[i].mTextureClient);

From a non-e10s run (but happens in either):

> ImageBridgeChild (28)#0	0x0000000103701d65 in mozilla::layers::ImageClientSingle::UpdateImage(mozilla::layers::ImageContainer*, unsigned int) at /Users/Jan/moz/mozilla-central/gfx/layers/client/ImageClient.cpp:164
> #1	0x000000010378288b in mozilla::layers::UpdateImageClientNow(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*) at /Users/Jan/moz/mozilla-central/gfx/layers/ipc/ImageBridgeChild.cpp:411
> #2	0x00000001037a51d2 in void DispatchToFunction<void (*)(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*), mozilla::layers::ImageClient*, nsRefPtr<mozilla::layers::ImageContainer> >(void (*)(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*), Tuple2<mozilla::layers::ImageClient*, nsRefPtr<mozilla::layers::ImageContainer> > const&) at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/tuple.h:458
> #3	0x00000001037a5168 in RunnableFunction<void (*)(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*), Tuple2<mozilla::layers::ImageClient*, nsRefPtr<mozilla::layers::ImageContainer> > >::Run() at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/task.h:433
> #4	0x00000001026996d0 in MessageLoop::RunTask(Task*) at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/message_loop.cc:364
> #5	0x0000000102699c4f in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/message_loop.cc:372
> #6	0x0000000102699e74 in MessageLoop::DoWork() at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/message_loop.cc:459
> #7	0x000000010269aae7 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/message_pump_default.cc:34
> #8	0x00000001026995b5 in MessageLoop::RunInternal() at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/message_loop.cc:234
> #9	0x00000001026994c5 in MessageLoop::RunHandler() at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/message_loop.cc:227
> #10	0x000000010269946d in MessageLoop::Run() at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
> #11	0x00000001026bebc9 in base::Thread::ThreadMain() at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/thread.cc:170
> #12	0x00000001026bfe0c in ThreadFunc(void*) at /Users/Jan/moz/mozilla-central/ipc/chromium/src/base/platform_thread_posix.cc:39
> #13	0x00007fff8dfd5899 in _pthread_body ()
> #14	0x00007fff8dfd572a in _pthread_start ()
> #15	0x00007fff8dfd9fc9 in thread_start ()

In e10s I sometimes get a crash as well in chrome process (this is with patch in Bug 1205164):

http://mxr.mozilla.org/mozilla-central/source/ipc/glue/Shmem.cpp?rev=aee0f61516c5&mark=322-322#314

   // if the segment isn't owned by the current process, these will
   // trigger SIGSEGV
 > char checkMappingFront = *reinterpret_cast<char*>(mData);

EXC_BAD_ACCESS (code=1, address=0x114fe6000)

> Thread 101#0	0x0000000101e5207d in mozilla::ipc::Shmem::AssertInvariants() const at /Users/Jan/moz/mozilla-central/ipc/glue/Shmem.cpp:322
> #1	0x0000000102fb8ad9 in unsigned long mozilla::ipc::Shmem::Size<char>() const at /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/gfx/thebes/../../dist/include/mozilla/ipc/Shmem.h:172
> #2	0x0000000104c08af4 in mozilla::ShmemPool::GetIfAvailable(unsigned long) at /Users/Jan/moz/mozilla-central/dom/media/systemservices/ShmemPool.cpp:47
> #3	0x0000000104c089b6 in mozilla::camera::CamerasParent::GetBuffer(unsigned long) at /Users/Jan/moz/mozilla-central/dom/media/systemservices/CamerasParent.cpp:193
> #4	0x0000000104c08c1d in mozilla::camera::CallbackHelper::DeliverFrame(unsigned char*, int, unsigned int, long long, long long, void*) at /Users/Jan/moz/mozilla-central/dom/media/systemservices/CamerasParent.cpp:205
> #5	0x0000000105eee25d in webrtc::ViEExternalRendererImpl::RenderFrame(unsigned int, webrtc::I420VideoFrame&) at /Users/Jan/moz/mozilla-central/media/webrtc/trunk/webrtc/video_engine/vie_renderer.cc:245
> #6	0x00000001060cff71 in webrtc::IncomingVideoStream::IncomingVideoStreamProcess() at /Users/Jan/moz/mozilla-central/media/webrtc/trunk/webrtc/modules/video_render/incoming_video_stream.cc:342
> #7	0x00000001060cfa05 in webrtc::IncomingVideoStream::IncomingVideoStreamThreadFun(void*) at /Users/Jan/moz/mozilla-central/media/webrtc/trunk/webrtc/modules/video_render/incoming_video_stream.cc:290
> #8	0x0000000105fc4907 in webrtc::ThreadPosix::Run() at /Users/Jan/moz/mozilla-central/media/webrtc/trunk/webrtc/system_wrappers/source/thread_posix.cc:365
> #9	0x0000000105fc43b5 in webrtc::StartThread(void*) at /Users/Jan/moz/mozilla-central/media/webrtc/trunk/webrtc/system_wrappers/source/thread_posix.cc:106
> #10	0x00007fff8dfd5899 in _pthread_body ()
> #11	0x00007fff8dfd572a in _pthread_start ()
> #12	0x00007fff8dfd9fc9 in thread_start ()

gcp, any idea what could be causing this?

Comment 22

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

There's also a 3.5 second delay for the video to update. Unsure if there's a way to improve on that.

Comment 23

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

Comment on attachment 8662163 [details]
MozReview Request: Bug 912342 - Move code MediaOperationTask from .h to .cpp

https://reviewboard.mozilla.org/r/19541/#review17505

Comment 24

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #22)
> There's also a 3.5 second delay for the video to update. Unsure if there's a
> way to improve on that.

3.5 seconds where?  That sounds very wrong

Comment 25

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

Your fiddle seems to change the resolution almost immediately for me on Linux with a Logitech 615 webcam. (on a debug build); e10s and non-e10s

Comment 26

[Gian-Carlo Pascutto [:gcp]]

>In e10s I sometimes get a crash as well in chrome process (this is with patch in Bug 1205164):
>gcp, any idea what could be causing this?

Well I did say I didn't want to remove the assert because it was showing a real bug, right? Your crasher seems like a direct result of the inconsistency the assert sees. I'll debug in 1205164.

Comment 27

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

(In reply to Randell Jesup [:jesup] from comment #24)
> 3.5 seconds where?  That sounds very wrong

It's less than 2 seconds in opt build. This is par for gUM on my OSX so maybe that's ok.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8662162 [details]
MozReview Request: Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

So we have the 'optional' because our webidl implementation follows the webidl spec "If the type of an argument is a dictionary type or a union type that has a dictionary type as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument MUST be specified as optional." but Media Capture and Streams spec doesn't, right?
So, please file a spec bug.

Comment 29

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

PR filed https://github.com/w3c/mediacapture-main/pull/245

Comment 30

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #27)
> (In reply to Randell Jesup [:jesup] from comment #24)
> > 3.5 seconds where?  That sounds very wrong
> 
> It's less than 2 seconds in opt build. This is par for gUM on my OSX so
> maybe that's ok.

Ok.  I think this is OS overhead, not ours.  Not a ton we can do about that unless we handle all downscaling ourselves.

Comment 31

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

Well the good news is that, as long as I don't attach the child process in the debugger then everything runs fine, so the asserts in comment 21 seems benign, and it seems it's the actual pausing in the debugger that is giving it hickups, and that was true for me with gUM as well, so I'm removing the dependency.

Comment 32

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

Comment on attachment 8662164 [details]
MozReview Request: Bug 912342 - Change capture resolution

Bug 912342 - Change capture resolution

Comment 33

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

Comment on attachment 8662164 [details]
MozReview Request: Bug 912342 - Change capture resolution

https://reviewboard.mozilla.org/r/19543/#review17601

Generally looks good, a few things left to wrap up

::: dom/media/MediaManager.cpp:339
(Diff revision 1)
> +          nsRefPtr<MediaManager> mgr = MediaManager::GetInstance();

Perhaps should consider having an nsRefPtr<...> mManager in this class and initialize it when we start an ApplyConstraints operation.  But I'm not sure it gains us anything.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:220
(Diff revision 1)
> +                                                          nsString())) { // XXX

XXX what?
Also, what do we want to do about Restart in the cases that fail this test?   Transform into Start()?  Throw an error?  LOG a warning?

Comment 34

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

https://reviewboard.mozilla.org/r/19543/#review17601

> XXX what?
> Also, what do we want to do about Restart in the cases that fail this test?   Transform into Start()?  Throw an error?  LOG a warning?

The XXX is the deviceId string, which has to be passed in since it's unique per origin. I need to fix MST to remember its origin-specific deviceId, and pass it in here, just in case some rocket scientist does .applyConstraints({ deviceId: someDifferentId }) expecting it to fail. Utterly useless, but needed for spec compliance I suppose.

As to the if-statement, I hope it's not possible to call applyConstraints before it has started? (I'll add an assert to test that). If it's stopped then a no-op seems right (once we add `track.getSettings()` - which I've punted on here - there'll be more code here to store the constraints that have been set).

But if ChooseCapability() fails, there's where I have more work to do. I was so excited to get this working I forgot about OverconstrainedError. Even gUM isn't returning it yet (Bug 1181896) because it's a bit of a pain [1]. But adding a new API with the wrong error is probably a no-go. 

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#overconstrainederror-object

Comment 35

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

Comment on attachment 8662163 [details]
MozReview Request: Bug 912342 - Move code MediaOperationTask from .h to .cpp

Bug 912342 - Move code MediaOperationTask from .h to .cpp

Comment 36

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

Comment on attachment 8662162 [details]
MozReview Request: Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Comment 37

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

Comment on attachment 8662164 [details]
MozReview Request: Bug 912342 - Change capture resolution

Bug 912342 - Change capture resolution

Comment 38

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

Attachment: MozReview Request: Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

Comment 39

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

Attachment: MozReview Request: Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

Comment 40

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

Attachment: MozReview Request: Bug 912342 - get Promise out.

Bug 912342 - get Promise out.

Comment 41

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

Comment on attachment 8662162 [details]
MozReview Request: Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

No change. Carrying forward r=smaug (patch was only r+'ed in bugzilla, not review board).

Comment 42

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

Comment on attachment 8662163 [details]
MozReview Request: Bug 912342 - Move code MediaOperationTask from .h to .cpp

Bug 912342 - Move code MediaOperationTask from .h to .cpp

Comment 43

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

Comment on attachment 8662162 [details]
MozReview Request: Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Comment 44

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

Comment on attachment 8662164 [details]
MozReview Request: Bug 912342 - Change capture resolution

Bug 912342 - Change capture resolution

Comment 45

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

Comment on attachment 8663446 [details]
MozReview Request: Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

Comment 46

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

Comment on attachment 8663447 [details]
MozReview Request: Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

Comment 47

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

Comment on attachment 8663448 [details]
MozReview Request: Bug 912342 - get Promise out.

Bug 912342 - get Promise out.

Comment 48

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

Comment on attachment 8662163 [details]
MozReview Request: Bug 912342 - Move code MediaOperationTask from .h to .cpp

Bug 912342 - Move code MediaOperationTask from .h to .cpp

Comment 49

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

Comment on attachment 8662162 [details]
MozReview Request: Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Comment 50

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

Comment on attachment 8662164 [details]
MozReview Request: Bug 912342 - Change capture resolution

Bug 912342 - Change capture resolution

Comment 51

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

Comment on attachment 8663446 [details]
MozReview Request: Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

Comment 52

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

Comment on attachment 8663447 [details]
MozReview Request: Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

Comment 53

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

Comment on attachment 8663448 [details]
MozReview Request: Bug 912342 - get Promise out.

Bug 912342 - get Promise out.

Comment 54

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

Comment on attachment 8662162 [details]
MozReview Request: Bug 912342 - Add MediaStreamTrack.applyConstraints webidl

Rebased to tip this time.

Comment 55

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

Comment on attachment 8663448 [details]
MozReview Request: Bug 912342 - get Promise out.

Bug 912342 - get Promise out.

Comment 56

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

Comment on attachment 8662164 [details]
MozReview Request: Bug 912342 - Change capture resolution

https://reviewboard.mozilla.org/r/19543/#review17771

::: dom/media/webrtc/MediaEngineTabVideoSource.cpp:299
(Diff revision 4)
> +  // TODO

Presume this is in a following patch

Comment 57

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

Comment on attachment 8663446 [details]
MozReview Request: Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

https://reviewboard.mozilla.org/r/19805/#review17773

Comment 58

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

(In reply to Randell Jesup [:jesup] from comment #56)
> Presume this is in a following patch

Yes, the last one.

Comment 59

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

Comment on attachment 8663447 [details]
MozReview Request: Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

https://reviewboard.mozilla.org/r/19807/#review17775

Comment 60

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

Comment on attachment 8663448 [details]
MozReview Request: Bug 912342 - get Promise out.

https://reviewboard.mozilla.org/r/19809/#review17777

Comment 61

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

Comment on attachment 8662164 [details]
MozReview Request: Bug 912342 - Change capture resolution

Bug 912342 - Change capture resolution

Comment 62

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

Comment on attachment 8663446 [details]
MozReview Request: Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

Bug 912342 - Pass in Audio/VideoDevice in place of Audio/VideoSource

Comment 63

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

Comment on attachment 8663447 [details]
MozReview Request: Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

Bug 912342 - Move code SelectSettings to MediaConstraintsHelper.

Comment 64

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

Comment on attachment 8663448 [details]
MozReview Request: Bug 912342 - get Promise out.

Bug 912342 - get Promise out.

Comment 65

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

Fixed some try errors. Trying again now.

Comment 66

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

Green try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2069a0548b0

If someone could check this in before tomorrow that would be great!

Comment 67

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

Note, requires check-in of Bug 1195951 as well (included in try).

Comment 68

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd1fcbc04d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3ef1ea74f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6cf02b174c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e01353044b85
https://hg.mozilla.org/integration/mozilla-inbound/rev/6124439438d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff3901d0989

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7bd1fcbc04d1
https://hg.mozilla.org/mozilla-central/rev/7b3ef1ea74f1
https://hg.mozilla.org/mozilla-central/rev/7e6cf02b174c
https://hg.mozilla.org/mozilla-central/rev/e01353044b85
https://hg.mozilla.org/mozilla-central/rev/6124439438d4
https://hg.mozilla.org/mozilla-central/rev/1ff3901d0989

Comment 70

[Boris Zbarsky [:bzbarsky]]

Why do you need the [Exposed=Window] here?  That's the default value anyway...

Comment 71

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

Same as silly whitespace, to keep webidl matching the spec literally. [1]

That seemed to matter to some people.

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamTrack

Comment 72

[Tom Brückner]

I have a question on this, since I could not find any documentation regarding applyConstraints() on the MDN wiki. (If there's another way to ask questions like this, please point me to the right place. Thx.)

My problem is: I am starting with a restricted resolution, let's call it "SD". It is defined like this:

  audio: true,
  video: {
    frameRate: 10,
    width: {
      max: 360
    }
  }

(Please note that I am using adapter.js to adjust constraints to the right format.)

This restricted resolution is set by getUserMedia(). This works and I get a small resolution with a slow frame-rate.

I now want to implement a toggle to a esolution that is defined as this (let's call it HD):

  audio: true,
  video: true

In Chrome I am doing this via calling gUM again, since there is no applyConstraints(). In Firefox, gUM does not work for this, the restricted video constraints seem to be "sticky". So I am using applyContraints() instead here. Both approaches work, and I get a higher resolution (640x480).

However, if I try to go back to the "SD" mode, it only works in Chrome. In Firefox, the higher resolution is "sticky", even if I call applyConstraints with the "SD" constraints again.

Why is this so? Is there any other way to restrict the resolution from a higher to a lower one "on the fly"?

Comment 73

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

see comment 72 (which perhaps should be in a new/different bug)

Comment 74

[Tom Brückner]

Thanks for your quick reaction. Some more info from my recent tests: I also tried switching with a simpler scenario, where both constraints just define width and height and nothing else. The problem also occurs in this case. On the second switch (from HD to SD), nothing happens.

Strange thing is, though, that I found this tutorial here:
https://tools.tutorialspoint.com:9001/demos/applyConstraints.html

There, the very same approach works. And I am pretty sure that I am doing it exactly the same way, by first calling .getVideoTracks() on the stream and then calling .applyConstraints() on all tracks (which is only one in my case). And it works from SD to HD as expected, but not the other way around.

Comment 75

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

Hi Tom, thanks for pointing out the missing applyConstraints docs on MDN! I've added dev-doc-needed.

For now, try the demo page in the URL field of this bug, and see http://stackoverflow.com/a/29359319/918910.

Bugzilla is not a place to address questions. Post further questions to http://stackoverflow.com tagged with [getUserMedia], or visit #media @ irc.mozilla.org if you use IRC. I lurk in both places.

Comment 76

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

I should also add that when I try https://jsfiddle.net/ftdmdkjt/ I'm able to go from SD to HD and back just fine, but this depends entirely on what modes your camera supports. See also bug 1271918 comment 1.