In multiple content processes case, there are multiple MediaManager / MediaEngineRemoteVideoSource in each process. The logic in ReevaluateAllocation() would fail to collect the constraints in another process. We need to move the logic to someone else. CamerasParent may be a choice, which is the only one singleton object which handles all requests from every content process's CamerasChild.

I wanted to put down some notes on overall direction here for context. We've been working on getting agreement with other vendors about how to solve the downscaling vs discovery dilemma in getUserMedia across browsers. Apparently there's less support for a "driver resolutions" discovery-model-ONLY than we previously thought [1]. That model let sites rely on OverconstrainedError to detect native/driver resolutions and frame rate capabilities. That said, we're not tossing our model entirely to embrace opaque downscaling, as even Chrome returns driver modes if you use {min, max} and leave out {ideal}. We probably won't rescale on {ideal} alone, but we may adopt downscaling on {exact} and basically never fail with OverconstrainedError unless the request exceeds max device abilities, like Chrome. But in the context of this bug, we've learned enough to know that the model where constraints can be used to detect camera use in other tabs is not desirable. In other words, we should independently downscale here to abstract away knowledge of other tabs. This should reduce the multiple content process problem down to using the highest resolution requested. I propose we sacrifice frameRate in these situations, to simplify and reduce information leakage. A lot of work went into constraints to solve the resolution vs frame-rate problem, but concurrent use is an edge-case, so things still work optimally in optimal (single-use) cases. I think that's the right trade-off. This way we won't need to lift the full constraints algorithm into the master process. In the interest of time, we can also leave the UpdateSingleSource model alone for now WITHIN the same content process, as it already picks the highest resolution requested. I think the plan is to consolidate downscaling in the track, so this should work. [1] https://github.com/w3c/mediacapture-main/issues/472#issuecomment-321267957

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review175382 Looks good, some nits on terminology, questions on pointers and lifetimes. Also a code comment on what it does and why would be good. ::: commit-message-52285:1 (Diff revision 1) > +Bug 1388667 - Normalize constraints from multiple content processes. r?jib I wouldn't use the words "normalize" or "constraints", since we're not moving constraints to the master process per comment 2 (NormalizedConstraintsSet is just a more invariant internal represention of constraints than the unpredictable webidl definition). ::: dom/media/systemservices/CamerasParent.cpp:42 (Diff revision 1) > #define LOG_ENABLED() MOZ_LOG_TEST(gCamerasParentLog, mozilla::LogLevel::Debug) > > namespace mozilla { > namespace camera { > > +std::map<void *, const char *> sDeviceNames; What if two devices have the same name? Can we find another way to distinguish devices? Is this module lacking a pattern for this? ::: dom/media/systemservices/CamerasParent.cpp:43 (Diff revision 1) > > namespace mozilla { > namespace camera { > > +std::map<void *, const char *> sDeviceNames; > +std::map<void *, webrtc::VideoCaptureCapability> sNormalizedConstraints; This appears to be a map of capabilities, not constraints. Maybe sAllRequestedCapabilities? Also, we should avoid pointers, especially void ones. ::: dom/media/systemservices/CamerasParent.cpp:826 (Diff revision 1) > + sDeviceNames.emplace(cap.VideoCapture().get(), cap.VideoCapture()->CurrentDeviceName()); > + sNormalizedConstraints.emplace(cap.VideoCapture().get(), capability); What's the lifetime of cap.VideoCapture()? How do we know this doesn't cause UAFs? ::: dom/media/systemservices/CamerasParent.cpp:833 (Diff revision 1) > + > + capability.width = 0; > + capability.height = 0; > + capability.maxFPS = 0; > + > + for (iter = sDeviceNames.begin(); iter != sDeviceNames.end(); ++iter) { for ( : ) ::: dom/media/systemservices/CamerasParent.cpp:835 (Diff revision 1) > + capability.width = capability.width > sNormalizedConstraints[iter->first].width ? > + capability.width : sNormalizedConstraints[iter->first].width; > + capability.height = capability.height > sNormalizedConstraints[iter->first].height ? > + capability.height : sNormalizedConstraints[iter->first].height; > + capability.maxFPS = capability.maxFPS > sNormalizedConstraints[iter->first].maxFPS ? > + capability.maxFPS : sNormalizedConstraints[iter->first].maxFPS; std::max()

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review175740 ::: dom/media/systemservices/CamerasParent.cpp:42 (Diff revision 1) > #define LOG_ENABLED() MOZ_LOG_TEST(gCamerasParentLog, mozilla::LogLevel::Debug) > > namespace mozilla { > namespace camera { > > +std::map<void *, const char *> sDeviceNames; Actually, this devicename is an unique ID, not a human readable text. I use the term devicename because of this function. http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc#36-39 I will change the variable's name to sDeviceUniqueIDs

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review175382 > This appears to be a map of capabilities, not constraints. Maybe sAllRequestedCapabilities? Also, we should avoid pointers, especially void ones. I will change the name to sAllRequestedCapabilities. The void pointer, which points to a VideoCaptureModule object, is only used as a key in this map. std::map won't dereference it so there won't be UAF concern. Before a VideoCaptureModule object is destructed (because the track is stopped or the external camera is removed), the entry which's key pointing to the object will be deleted in CamerasParent::StopCapture(). So the lifetime of this pointer won't exceed the lifetime of the object. > What's the lifetime of cap.VideoCapture()? How do we know this doesn't cause UAFs? cap.VideoCapture also points to a VideoCaptureModule object. The VideoCaptureModule object is hold by a refPtr in CaptureEntry [1]. It will not be destroyed as long as we hold the object cap. The original source code also calls cap.VideoCapture()-> StartCapture() [2] in the same scope. [1] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/dom/media/systemservices/VideoEngine.h#82 [2] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/dom/media/systemservices/CamerasParent.cpp#822

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review175382 > std::max() I think the logic I put here is totally wrong. For example, if the camera support VGA & 720p, and there are two requests 480x640 & 640x480. This logic here would produce 640x640, which is not a supported native resolution. Instead, we should configure camera to output 720p and downscale for both requests. We cannot come out this resolution via only the information of all requests. We need to know all supported capabilities in CamerasParent.

If it helps, capabilities originate in CamerasParent [1] [1] https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/dom/media/systemservices/CamerasParent.cpp#534-537

The patch won't be landed immediately. It will be landed with bug 1388219.

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review178266 ::: dom/media/webrtc/MediaTrackConstraints.cpp:410 (Diff revision 2) > template<class ValueType, class NormalizedRange> > /* static */ uint32_t > MediaConstraintsHelper::FitnessDistance(ValueType aN, > const NormalizedRange& aRange) > { > - if (aRange.mMin > aN || aRange.mMax < aN) { > + if (aRange.mMin > aN) { This change looks correct, since we'll only want to fail with OverconstrainedError on {min} now, not {max}. ::: dom/media/webrtc/MediaTrackConstraints.cpp:413 (Diff revision 2) > + // We prefer larger resolution because now we support downscaling > if (aN == aRange.mIdeal.valueOr(aN)) { > return 0; > - } > + } else if (aN > aRange.mIdeal.value()) { > - return uint32_t(ValueType((std::abs(aN - aRange.mIdeal.value()) * 1000) / > + return uint32_t(ValueType((std::abs(aN - aRange.mIdeal.value()) * 1000) / > - std::max(std::abs(aN), std::abs(aRange.mIdeal.value())))); > + std::max(std::abs(aN), std::abs(aRange.mIdeal.value())))); > + } else { > + return (UINT32_MAX / 2) + uint32_t(ValueType((std::abs(aN - aRange.mIdeal.value()) * 1000) / > + std::max(std::abs(aN), std::abs(aRange.mIdeal.value())))); > + } However, I'm less sure we want to alter the rest of the fitness distance algorithm, as we MAY want to prefer finding driver resolutions when not forced to rescale with min/max/exact constraints. E.g. Chrome allows finding optimal (native/driver) modes like this: {width: {min: 640, max: 1920}} = "I want an optimal mode between 640 and 1920" --> 640 However, it doesn't give users much help finding more than one in that range. I.e. it won't let you say: {width: {min: 640, ideal: 1024, max: 1920}} = "I want an optimal mode around 1024, min 640, max 1920" Instead it rescales the instant it sees an ideal value: {width: {min: 640, ideal: 1024, max: 1920}} = "give me 1024" --> 1280 downscaled to 1024 {width: {ideal: 1024}} = "give me 1024" --> 1280 downscaled to 1024 {width: 1024} = "give me 1024" --> 1280 downscaled to 1024 {width: {exact: 1024}} = "give me 1024" --> 1280 downscaled to 1024 That's four ways to say "give me 1024", which seems a bit redundant imho. Not sure that's what the spec intended. Another approach we could take is only downscale when the app forces us to: {width: {min: 640, ideal: 1024, max: 1920}} = "I want an optimal mode around 1024, min 640, max 1920" --> 1280 {width: {ideal: 1024}} = "I want an optimal mode around 1024" --> 1280 {width: 1024}} = "I want an optimal mode around 1024" --> 1280 {width: {exact: 1024}} = "give me 1024" --> 1280 downscaled to 1024 I'm not sure yet if this approach is without problems, but it seems a compromise between current Chrome vs. Firefox and Edge behavior.

Not sure which approach we should take yet. Open to input. Just wanted to write that down somewhere.

You are right, we shouldn't alter the fitness distance algorithm including the change which removes "aRange.mMax < aN" because it will also impact the decision in driver resolution mode. Here is my plan: I will leave the new constraint behavior job in bug 1286945. We don't need to hurry to lock down the final decision with other vendors. I also don't expect there will be any change in 57. In this bug and bug 1388219, I only want to address the issue of 1) multiple gUMs in the same content process 2) multiple gUMs in different content processes]. We should still provide native/driver resolution only. We downscale the camera output to one of the native/driver resolutions if necessary. In current behavior, we merge the multiple constraints first, then find out a resolution with shortest fitness distance. For example, when there are two requests 640x480 and 480x640, we find a resolution which is most close to 640x640. With the ability of downscaling, we can change the behavior to: 1. Use choosecapability to find out a resolution with shortest distance for 640x480, e.g., 640x480. 2. Use choosecapability to find out a resolution with shortest distance for 480x640 e.g., 640x480. 3. Find out a smallest native resolution which is larger than 640x480 and 640x480. In this way, we can ensure in most cases, the behavior of single gUM and multiple gUMs would be the same. A tab can not ear drop other tabs' status.

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review179074 Still needs some changes I think. ::: dom/media/systemservices/CamerasParent.cpp:42 (Diff revisions 1 - 3) > -std::map<void *, const char *> sDeviceNames; > -std::map<void *, webrtc::VideoCaptureCapability> sNormalizedConstraints; > +std::map<void *, const char *> sDeviceUniqueIDs; > +std::map<void *, webrtc::VideoCaptureCapability> sAllRequestedCapabilities; I'm still not happy about using memory pointers as indexes. This assumes something about lifetimes of the objects passed in. At least in theory, a capture engine could be freed, and a new one created in the same place in memory, yielding the wrong id here. Instead of mapping an id to a memory pointer, we should just use the id itself as index. That's what they're for AFAICT. They appear to come from the engine itself [1], and seem obtainable directly without a map? Worst case, can we pass it in? [1] https://dxr.mozilla.org/mozilla-central/rev/1b4c59eef820b46eb0037aca68f83a15088db45f/dom/media/systemservices/CamerasParent.cpp#603,626 ::: dom/media/systemservices/CamerasParent.cpp:44 (Diff revisions 1 - 3) > namespace mozilla { > namespace camera { > > -std::map<void *, const char *> sDeviceNames; > -std::map<void *, webrtc::VideoCaptureCapability> sNormalizedConstraints; > +std::map<void *, const char *> sDeviceUniqueIDs; > +std::map<void *, webrtc::VideoCaptureCapability> sAllRequestedCapabilities; > +std::map<std::string, std::map<uint32_t, webrtc::VideoCaptureCapability>> sAllCandidateCapabilities; Any reason not to use nsCString here? ::: dom/media/systemservices/CamerasParent.cpp:47 (Diff revisions 1 - 3) > +CalculateDistance(int32_t candidate, int32_t requested) > +{ > + if (candidate >= requested) { > + return (candidate - requested) * 1000 / std::max(candidate, requested); > + } else { > + return (UINT32_MAX / 2) + (requested - candidate) * 1000 / std::max(candidate, requested); Maybe add a code comment here on why we strongly prefer higher resolutions? I'm actually not sure if falling back to a lower resolution is ok in all cases, since it might violate a {min: x} constraint, but I think I'm ok for now since we don't have a good plan for what to do in that edge-case that wouldn't involve allowing JS to detect other tabs (though they can detect it regardless I suppose by seeing their width or height constraint violated by polling getSettings()). This may be a use-case for firing the overconstrainederror event, which we don't implement yet, but let's defer that to a future bug perhaps, if ever. I'm not sure firing rare unexpected events in rare circumstances is ever a good idea. In most cases, people will be asking for same aspect, so I think we're fine. ::: dom/media/systemservices/CamerasParent.cpp:845 (Diff revisions 1 - 3) > if (aCapEngine == CameraEngine) { > - auto iter = sDeviceNames.find(cap.VideoCapture().get()); > - MOZ_ASSERT(iter == sDeviceNames.end()); > + auto deviceUniqueID = sDeviceUniqueIDs.find(cap.VideoCapture().get()); > + MOZ_ASSERT(deviceUniqueID == sDeviceUniqueIDs.end()); I may not be understanding the invariant here: Can StartCapture() really not be called more than once concurrently on the same capture engine? We're in the parent process here, are the ids unique per content process? A comment would be good. I see sDeviceUniqueIDs.erase is used in StopCapture, but is that called if there's an error below? Are we sure StopCapture() is always called in all cases? I worry there might be an edge-case error situation that might trigger this MOZ_ASSERT and cause crashes in the debug build. Is there a less fragile way to do this, like store the id(s?) on the object? ::: dom/media/systemservices/CamerasParent.cpp:869 (Diff revisions 1 - 3) > + // The first priority is finding a suitable resolution. > + // So here we raise the weight of width and height > + uint64_t distance = 10 * uint64_t(CalculateDistance(candidateCapability.second.width, capability.width)) + > + 10 * uint64_t(CalculateDistance(candidateCapability.second.height, capability.height)) + > + uint64_t(CalculateDistance(candidateCapability.second.maxFPS, capability.maxFPS)); Since CalculateDistance() itself adds half a UINT32_MAX (which is larger than 10) if candidate maxFPS is less than desired, this does not seem to match what the comment here says we're doing. Maybe a separate CalculateResolutionDistance() ?

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review179074 > Maybe add a code comment here on why we strongly prefer higher resolutions? > > I'm actually not sure if falling back to a lower resolution is ok in all cases, since it might violate a {min: x} constraint, but I think I'm ok for now since we don't have a good plan for what to do in that edge-case that wouldn't involve allowing JS to detect other tabs (though they can detect it regardless I suppose by seeing their width or height constraint violated by polling getSettings()). This may be a use-case for firing the overconstrainederror event, which we don't implement yet, but let's defer that to a future bug perhaps, if ever. I'm not sure firing rare unexpected events in rare circumstances is ever a good idea. > > In most cases, people will be asking for same aspect, so I think we're fine. I meant, in most cases, people will be asking for the same *orientation* here, not aspect.

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review179074 > I'm still not happy about using memory pointers as indexes. This assumes something about lifetimes of the objects passed in. > > At least in theory, a capture engine could be freed, and a new one created in the same place in memory, yielding the wrong id here. > > Instead of mapping an id to a memory pointer, we should just use the id itself as index. That's what they're for AFAICT. They appear to come from the engine itself [1], and seem obtainable directly without a map? Worst case, can we pass it in? > > > [1] https://dxr.mozilla.org/mozilla-central/rev/1b4c59eef820b46eb0037aca68f83a15088db45f/dom/media/systemservices/CamerasParent.cpp#603,626 > At least in theory, a capture engine could be freed, and a new one created in the same place in memory yes, that's a risk. In this patch, we use an assert to detect this case. MOZ_ASSERT(deviceUniqueID == sDeviceUniqueIDs.end()); > we should just use the id itself as index. We cannot use id as indexes because there are multiple VideoCaptureModules in different content processes. Assuming all gUMs (in different processes) request the same camera with different capabilities, one id will map to multiple capabilities. When one of these mediastreams is stopped, we don't know which capability to remove. > I'm still not happy about using memory pointers as indexes. Using memory pointers as indexes is indeed awkward, I am looking for better substituttion. What about assigning an unique id to a VideoCaptureModule? > Any reason not to use nsCString here? I am not sure if nsCString can be used as key of std::map, which demands the class overloading some opearators. std::string definitely works well with std::map. Do you prefer using nsCString? > I meant, in most cases, people will be asking for the same *orientation* here, not aspect. No problem, I will add a comment here. > I may not be understanding the invariant here: Can StartCapture() really not be called more than once concurrently on the same capture engine? We're in the parent process here, are the ids unique per content process? A comment would be good. > > I see sDeviceUniqueIDs.erase is used in StopCapture, but is that called if there's an error below? Are we sure StopCapture() is always called in all cases? I worry there might be an edge-case error situation that might trigger this MOZ_ASSERT and cause crashes in the debug build. Is there a less fragile way to do this, like store the id(s?) on the object? > Can StartCapture() really not be called more than once concurrently on the same capture engine? http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#289-294 StartCapture() may be called twice, but there is always a pairing StopCapture(). > I worry there might be an edge-case error situation that might trigger this MOZ_ASSERT and cause crashes in the debug build. I have verified the patch in some cases like 1) disconnecting an external webcam or 2) close Firefox during gUM streaming. But indeed, there may be some corner cases I didn't cover. This assert actualy is put to detect these corner cases, if there is no any assert triggered in nightly debug build, we will be more confident that there is no such corner case. Maybe we can land the code after 57 moves to beta? > like store the id(s?) on the object Do you mean storing the id on the object VideoCaptureModule? VideoCaptureModule already store id [1], but if we want to retrive it, we need to dereference the key (memory pointer), or we need to use refptr to hold the object VideoCaptureModule. I worry that may impact the lifecycle of VideoCaptureModule and cause some deadlock or memory leakage. [1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc#36-39 > Since CalculateDistance() itself adds half a UINT32_MAX (which is larger than 10) if candidate maxFPS is less than desired, this does not seem to match what the comment here says we're doing. Maybe a separate CalculateResolutionDistance() ? Ah, right, I will add a separate distance calculation for fps.

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review179074 > > At least in theory, a capture engine could be freed, and a new one created in the same place in memory > > yes, that's a risk. In this patch, we use an assert to detect this case. > MOZ_ASSERT(deviceUniqueID == sDeviceUniqueIDs.end()); > > we should just use the id itself as index. > > We cannot use id as indexes because there are multiple VideoCaptureModules in different content processes. > Assuming all gUMs (in different processes) request the same camera with different capabilities, one id will map to multiple capabilities. When one of these mediastreams is stopped, we don't know which capability to remove. > > I'm still not happy about using memory pointers as indexes. > > Using memory pointers as indexes is indeed awkward, I am looking for better substituttion. > What about assigning an unique id to a VideoCaptureModule? Ok so you're tracking requested capabilities per `cap.VideoCapture()`, and you're saying that object is different per content-process this request comes from. Where I'm lost is: You get `cap` from `engine->WithEntry(capnum, ...)`, which AFAICT is just a lookup [1] of capnum. But is `capnum` unique per requesting process? If yes, why not use capnum as index instead? If no, how can this object be unique per requesting process? [1] https://dxr.mozilla.org/mozilla-central/rev/04b6be50a2526c7a26a63715f441c47e1aa1f9be/dom/media/systemservices/VideoEngine.cpp#168 > I am not sure if nsCString can be used as key of std::map, which demands the class overloading some opearators. > std::string definitely works well with std::map. Do you prefer using nsCString? Not me personally, but Mozilla does. I do see uses of std::map<nsCString in the tree, so I assume it works [1]. There are even mozilla-versions of std::map [2] if you want. [1] https://dxr.mozilla.org/mozilla-central/search?q=std%3A%3Amap%3CnsCString&redirect=false [2] https://dxr.mozilla.org/mozilla-central/rev/04b6be50a2526c7a26a63715f441c47e1aa1f9be/dom/media/MediaManager.h#321-322

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #17) > Where I'm lost is: You get `cap` from `engine->WithEntry(capnum, ...)`, > which AFAICT is just a lookup [1] of capnum. But is `capnum` unique per > requesting process? > > If yes, why not use capnum as index instead? You are right, capnum is a unique ID. We can use it as index. [1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/media/systemservices/VideoEngine.cpp#180 > Not me personally, but Mozilla does. > > I do see uses of std::map<nsCString in the tree, so I assume it works [1]. > There are even mozilla-versions of std::map [2] if you want. Great! then I will switch to nsCString.

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review180492 Great! Just minor nits, and I did a wordwrap pass. ::: dom/media/systemservices/CamerasParent.cpp:44 (Diff revisions 3 - 4) > namespace mozilla { > namespace camera { > > -std::map<void *, const char *> sDeviceUniqueIDs; > -std::map<void *, webrtc::VideoCaptureCapability> sAllRequestedCapabilities; > -std::map<std::string, std::map<uint32_t, webrtc::VideoCaptureCapability>> sAllCandidateCapabilities; > +std::map<uint32_t, const char *> sDeviceUniqueIDs; > +std::map<uint32_t, webrtc::VideoCaptureCapability> sAllRequestedCapabilities; > +std::map<nsCString, std::map<uint32_t, webrtc::VideoCaptureCapability>> sAllCandidateCapabilities; wordwrap ::: dom/media/systemservices/CamerasParent.cpp:55 (Diff revisions 3 - 4) > + // which is larger than all requested capabilities. > + // Then we can use down-scaling to fulfill each request. > if (candidate >= requested) { > return (candidate - requested) * 1000 / std::max(candidate, requested); > } else { > return (UINT32_MAX / 2) + (requested - candidate) * 1000 / std::max(candidate, requested); wordwrap ::: dom/media/systemservices/CamerasParent.cpp:576 (Diff revisions 3 - 4) > if (auto devInfo = engine->GetOrCreateVideoCaptureDeviceInfo()){ > error = devInfo->GetCapability(unique_id.get(), num, webrtcCaps); > } > > if (!error && aCapEngine == CameraEngine) { > - auto iter = sAllCandidateCapabilities.find(std::string(unique_id.get())); > + auto iter = sAllCandidateCapabilities.find(nsCString(unique_id.get())); s/nsCString(unique_id.get())/unique_id/ ::: dom/media/systemservices/CamerasParent.cpp:580 (Diff revisions 3 - 4) > if (!error && aCapEngine == CameraEngine) { > - auto iter = sAllCandidateCapabilities.find(std::string(unique_id.get())); > + auto iter = sAllCandidateCapabilities.find(nsCString(unique_id.get())); > if (iter == sAllCandidateCapabilities.end()) { > std::map<uint32_t, webrtc::VideoCaptureCapability> candidateCapabilities; > candidateCapabilities.emplace(num, webrtcCaps); > - sAllCandidateCapabilities.emplace(std::string(unique_id.get()), candidateCapabilities); > + sAllCandidateCapabilities.emplace(nsCString(unique_id.get()), candidateCapabilities); just unique_id ::: dom/media/systemservices/CamerasParent.cpp:844 (Diff revisions 3 - 4) > if (self->EnsureInitialized(aCapEngine)) { > cbh = self->mCallbacks.AppendElement( > new CallbackHelper(static_cast<CaptureEngine>(aCapEngine), capnum, self)); > > engine = self->mEngines[aCapEngine]; > - engine->WithEntry(capnum, [&aCapEngine, &engine, &error, &ipcCaps, &cbh](VideoEngine::CaptureEntry& cap) { > + engine->WithEntry(capnum, [&capnum, &aCapEngine, &engine, &error, &ipcCaps, &cbh](VideoEngine::CaptureEntry& cap) { wordwrap ::: dom/media/systemservices/CamerasParent.cpp:862 (Diff revisions 3 - 4) > capability.width = std::max(capability.width, sAllRequestedCapabilities[it.first].width); > capability.height = std::max(capability.height, sAllRequestedCapabilities[it.first].height); > capability.maxFPS = std::max(capability.maxFPS, sAllRequestedCapabilities[it.first].maxFPS); wordwrap ::: dom/media/systemservices/CamerasParent.cpp:880 (Diff revisions 3 - 4) > - uint64_t distance = 10 * uint64_t(CalculateDistance(candidateCapability.second.width, capability.width)) + > - 10 * uint64_t(CalculateDistance(candidateCapability.second.height, capability.height)) + > - uint64_t(CalculateDistance(candidateCapability.second.maxFPS, capability.maxFPS)); > + uint64_t distance = uint64_t(ResolutionFeasibilityDistance(candidateCapability.second.width, capability.width)) + > + uint64_t(ResolutionFeasibilityDistance(candidateCapability.second.height, capability.height)) + > + uint64_t(FeasibilityDistance(candidateCapability.second.maxFPS, capability.maxFPS)); wordwrap ::: dom/media/systemservices/CamerasParent.cpp:896 (Diff revisions 3 - 4) > > error = cap.VideoCapture()->StartCapture(capability); > > if (!error) { > engine->Startup(); > cap.VideoCapture()->RegisterCaptureDataCallback(static_cast<rtc::VideoSinkInterface<webrtc::VideoFrame>*>(*cbh)); wordwrap

Shall we land this patch first? Even without re-scaling, the patch will make the behavior of multiple process e10s identical to the behavior of single process e10s.

Will the modification support the behavior like chrome : useing getusermedia to control the frame size and the frame rate ? It sounds a great feature.

Sorry for the delay. With the limited testing we have of constraints, with bug 1088621 still not landed, we should wait until 58 to land this imho.

(In reply to xpeng from comment #23) > Will the modification support the behavior like chrome : useing getusermedia > to control the frame size and the frame rate ? It sounds a great feature. If by "control" you mean indiscriminately downscale resolution and decimate frame rates in software, then see comment 10 and bug 1286945. We're moving closer to Chrome here at least. If instead you mean "control" the resolution output abilities and framerate output abilities of the hardware, as offered in drivers, then you can already do that in Firefox today, and arguably, your abilities to do so with precision (avoiding software downscaling and decimating) will be somewhat diminished as we chase the first definition of "control".

That's what i mean :"If by "control" you mean indiscriminately downscale resolution and decimate frame rates in software, then see comment 10 and bug 1286945. We're moving closer to Chrome here at least."

Mass change P1->P2 to align with new Mozilla triage process

Revision 5-6: rebase

Comment on attachment 8898649 [details] Bug 1388667 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/170020/#review199480 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/systemservices/CamerasParent.cpp:55 (Diff revision 6) > + // The purpose of this function is to find a smallest resolution > + // which is larger than all requested capabilities. > + // Then we can use down-scaling to fulfill each request. > + if (candidate >= requested) { > + return (candidate - requested) * 1000 / std::max(candidate, requested); > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Revision 6-7: fix static analysis bug

Pushed by mchiang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d057ff6cbcfb Set the highest resolution requested from multiple content processes. r=jib