Currently we offer the same resolution to mulitple getUserMedia MediaStream with different resolution constraints. We need to support down-scaling per track to meet each different constraint.

The effort is more than I expected. I think our design in MediaManager / MediaEngineVideoSource need some refactoring for multiple content processes. For example, the code that makes the screensharing/camera windows move with each other (https://dxr.mozilla.org/mozilla-central/rev/47248637eafa9a38dade8dc3aa6c4736177c8d8d/dom/media/webrtc/MediaEngine.h#183-192) won't allocate the larger resolution in this case. Instead, the later gUM always wins because there are two MediaManager / MediaEngineVideoSource in two different content processes now. Each MediaManager / MediaEngineVideoSource is not aware of the existence of each other.

This is an assigned P1 bug without activity in two weeks. If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword. Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

This bug is pending on bug 1388667. We are working on bug 1388667 now.

Comment on attachment 8904978 [details] Bug 1388219 - Set the highest resolution requested from multiple content processes. carry r+ from bug 1388667 comment 20

These patches are for preview. I will add more comments and avoid using memory pointer in mHandles.

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review182146 We will do re-scaling in NotifyPull() because this is the place we know which SourceMediaStream is pulling the data and what the target resolution we should provide according to its constraint. However, we cannot re-scale the frame easily if it has been packed as a layers::PlanarYCbCrImage. So we move the packing snippet to this function. Then we can re-scale the frame before packing it to a layers::PlanarYCbCrImage.

Comment on attachment 8904985 [details] Bug 1388219 - down scale camera output frame to the target capability. https://reviewboard.mozilla.org/r/176828/#review182148 Currently we merge all constraints first. Then find a capability with the shortest fitness distance from the merged constraint. If we want to re-scale, we need to know the target capability for each GUM request separately. So here I added a nsTArray mTargetapabilities to record each of them.

Comment on attachment 8904986 [details] Bug 1388219 - down scale camera output frame to the target capability. https://reviewboard.mozilla.org/r/176830/#review182156 In this patch, we move the re-scaling snippet from desktop_capture_impl.cc to MediaEngineRemoteVideoSource::NotifyPull and make it common used by both camera and desktop sharing case.

Move some changes from part 4 (down scale...) to part 3 (add a nsTArray...)

avoid using memory pointer as nsTArray (mHandleIds) element.

Please stop reviewing these patches. There is a serious performance downgrade. I will refactor my changes and generate a new patch.

There are some build error and failed test cases found in try server [1]. The latest revision fixes these issues. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa45f5dc7404d3a3b2f40000543e147ed8c4175

Fix memory leak found in asan build.

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

Comment on attachment 8904978 [details] Bug 1388219 - Set the highest resolution requested from multiple content processes. https://reviewboard.mozilla.org/r/176820/#review186268 I'm confused. Isn't this the same patch I already r+ed in bug 1388667? To avoid this, request review of a subset of commits in your queue, using: hg push -r firstcommit::lastcommit review

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review186272 I hope to finish review tomorrow. Just wanted to leave the comments I have so far. ::: dom/media/webrtc/MediaEngine.h:225 (Diff revision 3) > { > public: > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AllocationHandle); > protected: > ~AllocationHandle() {} > + static uint32_t sId; Should we use uint64_t here, like we do for windowIds? ::: dom/media/webrtc/MediaEngine.h:416 (Diff revision 3) > if (aConstraintsUpdate) { > allConstraints.AppendElement(aConstraintsUpdate); > + updatedConstraint.AppendElement(aConstraintsUpdate); > } else if (aHandle) { > // In the case of AddShareOfSingleSource, the handle isn't registered yet. > allConstraints.AppendElement(&aHandle->mConstraints); > + updatedConstraint.AppendElement(&aHandle->mConstraints); > + } else { > + updatedConstraint.AppendElements(allConstraints); ReevaluateAllocation() is called from three places [1], with different null args: 1. from Allocate(): ReevaluateAllocation(handle, nullptr, ... 2. from Restart(): ReevaluateAllocation(handle, &constraints, ... 3. from Deallocate(): ReevaluateAllocation(nullptr, nullptr, ... where 1 is new gUM, 2 is applyConstraints, and 3 is track.stop(). On track.stop() you're effectively setting newConstraint to netConstraints here, the max-not-average of all remaining constraints, which seems wrong. It may end up not mattering, since the track is stopped, but seems confusing, when you probably could pass in empty constraints instead. [1] http://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla17MediaEngineSource20ReevaluateAllocationEPNS0_16AllocationHandleEPNS_21NormalizedConstraintsERKNS_16MediaEnginePrefsERK9nsTStringIDsEPPKc&redirect=false ::: dom/media/webrtc/MediaEngine.h:427 (Diff revision 3) > NormalizedConstraints netConstraints(allConstraints); > if (netConstraints.mBadConstraint) { > *aOutBadConstraint = netConstraints.mBadConstraint; > return NS_ERROR_FAILURE; > } This merge constructor resolves inherently conflicting constraints by returning an error. E.g. {width: {max: 640}} and {width: {min: 1024}} // OverconstrainedError but with downscaling, these are no longer conflicts. It might be time to abandon the UpdateSingleSource abstraction (for video) here. ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:306 (Diff revision 3) > case kStarted: > + { Nit: brace at end case kStarted: { break; } https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures ::: dom/media/webrtc/MediaTrackConstraints.cpp:435 (Diff revision 3) > + return (UINT32_MAX / 2) + uint32_t(ValueType((std::abs(aN - aRange.mIdeal.value()) * 1000) / > + std::max(std::abs(aN), std::abs(aRange.mIdeal.value())))); (UINT32_MAX/2) is way too large a number to return here, given that distances are added together, and UINT32_MAX means infinity (OverconstrainedError). Also, considering that other constraints like aspectRatio (not implemented yet) might contribute 1*1000 to the distance, it may be wrong to weigh lower resolutions this unfavorably. Thought hard to tell before having implemented aspectRatio perhaps.

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review187790 Rest looks good. r- for the things I mentioned in comment 36. Basically, when I added the UpdateSingleSource abstraction in the base class, I envisioned that subclasses for which this abstraction was too limited, could instead implement their code directly in Allocate(), Restart() and Deallocate() instead. If you find having a common function is still helpful, then I won't mind. The important piece is to remove the netConstraints functionality now that different dimensions and frameRates can co-exist. ::: dom/media/webrtc/MediaEngineCameraVideoSource.h:27 (Diff revision 3) > +enum DistanceCalculation { > + kFitness, > + kFeasibility > +}; This may be a good place for a comment describing the difference between these two. ::: dom/media/webrtc/MediaEngineCameraVideoSource.h:96 (Diff revision 3) > + uint32_t GetDistance(const webrtc::CaptureCapability& aCandidate, > + const NormalizedConstraintSet &aConstraints, > + const nsString& aDeviceId, > + const DistanceCalculation aCalculate) const; Nit: style in this file is to align indent with first arg

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review187790 Forgive me, but I forget if we still need netConstraints to calculate the actual camera dimensions large enough to encompass all requests (in this process), or whether this is handled sufficiently covered by your new code in CamerasParent now. If it's the latter then hopefully the netConstraints abstraction can disappear completely here. If not, the important part to remove is the failing on conflicting (bad) constraints.

Comment on attachment 8904985 [details] Bug 1388219 - down scale camera output frame to the target capability. https://reviewboard.mozilla.org/r/176828/#review186292 This is exciting! I have some questions about whether some assumptions copied from screensharing still hold for camera. r- for those. I also need to test next. I'm also trying to think of a way to test this. We should be able to open two gUM streams now from the same screenshare and be able to apply different constraints, and see individual resolutions and frame rates, right? ::: dom/media/webrtc/MediaEngineCameraVideoSource.h:133 (Diff revision 6) > // All the mMonitor accesses are from the child classes. > - Monitor mMonitor; // Monitor for processing Camera frames. > + Monitor mMonitor, mBufMonitor; // Monitor for processing Camera frames. Nit: confusing which monitor the comment refers to ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp (Diff revision 6) > - MonitorAutoLock lock(mMonitor); > if (mState != kStarted) { Doesn't mMonitor protect mState? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:449 (Diff revision 6) > - layers::PlanarYCbCrData data; > - RefPtr<layers::PlanarYCbCrImage> image; > - { > - // We grab the lock twice, but don't hold it across the (long) CopyData > - MonitorAutoLock lock(mMonitor); > + for (int i = 0; i < (int) mSources.Length(); i++ ) { > + int32_t req_max_width = mTargetCapabilities[i].width & 0xffff; > + int32_t req_max_height = mTargetCapabilities[i].height & 0xffff; > + int32_t req_ideal_width = (mTargetCapabilities[i].width >> 16) & 0xffff; > + int32_t req_ideal_height = (mTargetCapabilities[i].height >> 16) & 0xffff; This part is a bit confusing. Looks copied from screensharing. Doesn't it assume max width and ideal width were crammed into the width field, and ditto for height, not just for screensharing, but for camera as well? This was a kludge done for screensharing where the actual source dimensions were not known until the frames come in. It was also necessary because with screensharing the source dimensions and even aspect can change dramatically, e.g. with window sharing when an end-user resizes a window. But where are we're cramming max and ideal values in for camera? I only see it for screensharing. [1] [1] http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#472-474,480-483 ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:474 (Diff revision 6) > + i420Buffer = webrtc::I420Buffer::Create(mWidth, mHeight, mWidth, > + (mWidth + 1) / 2, (mWidth + 1) / 2); nit: indent+1 ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:500 (Diff revision 6) > + if (!frame) > - return 0; > + return 0; {} ::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc (Diff revision 6) > - // scale to average of portrait and landscape > - float scale_width = (float)dst_width / (float)target_width; > - float scale_height = (float)dst_height / (float)target_height; > - float scale = (scale_width + scale_height) / 2; > - dst_width = (int)(scale * target_width); > - dst_height = (int)(scale * target_height); > - > - // if scaled rectangle exceeds max rectangle, scale to minimum of portrait and landscape Are we sure the new rescaling code covers both the needs of screensharing and camera? As I recall, there were some edge-cases with e.g. window-screensharing where users resize windows from landscape to portrait etc. and also possibly larger than (optional) max constraints. I can't follow the math to see if those properties are preserved. Also, our existing support for screensharing constraints has been to rescale to ideal values, which differs from what I imagined for cameras in bug 1388667 comment 10. Which way does it work? I'll try to have played with the patch before reviewing again. We should also ponder whether the different needs from screensharing and camera outweighs having rescaling behave consistently between the two types of media.

Comment on attachment 8904985 [details] Bug 1388219 - down scale camera output frame to the target capability. https://reviewboard.mozilla.org/r/176828/#review186292 > This part is a bit confusing. Looks copied from screensharing. > > Doesn't it assume max width and ideal width were crammed into the width field, and ditto for height, not just for screensharing, but for camera as well? This was a kludge done for screensharing where the actual source dimensions were not known until the frames come in. It was also necessary because with screensharing the source dimensions and even aspect can change dramatically, e.g. with window sharing when an end-user resizes a window. > > But where are we're cramming max and ideal values in for camera? I only see it for screensharing. [1] > > [1] http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#472-474,480-483 These logic is designed for both desktop sharing and camera cases. We only cram max and ideal values for screen sharing. For camera, both req_ideal_width and req_ideal_height will be 0. int32_t dst_width = std::min(req_ideal_width > 0 ? req_ideal_width : mWidth, dest_max_width); int32_t dst_height = std::min(req_ideal_height > 0 ? req_ideal_height : mHeight, dest_max_height); And then we will set dst_width to std::min(mWidth, dest_max_width) and dst_height to std::min(mHeight, dest_max_height) Maybe I should add more comments here to avoid confusion. > Are we sure the new rescaling code covers both the needs of screensharing and camera? > > As I recall, there were some edge-cases with e.g. window-screensharing where users resize windows from landscape to portrait etc. and also possibly larger than (optional) max constraints. I can't follow the math to see if those properties are preserved. > > Also, our existing support for screensharing constraints has been to rescale to ideal values, which differs from what I imagined for cameras in bug 1388667 comment 10. > > Which way does it work? I'll try to have played with the patch before reviewing again. > > We should also ponder whether the different needs from screensharing and camera outweighs having rescaling behave consistently between the two types of media. Yes, I run some basic testing. Resizing windows from landscape to portrait works fine, even the window size become larger than the max constraint (downscaling will shrink the size to match the max constraint). bug 1388667 comment 10 will be addressed in bug 1286945.

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review206010 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:66 (Diff revision 4) > + const nsString& aDeviceId, > + const DistanceCalculation aCalculate) const > +{ > + if (aCalculate == kFeasibility) > + return GetFeasibilityDistance(aCandidate, aConstraints, aDeviceId); > + else Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] ::: dom/media/webrtc/MediaTrackConstraints.cpp:431 (Diff revision 4) > + return UINT32_MAX; > + } > + // We prefer larger resolution because now we support downscaling > + if (aN == aRange.mIdeal.valueOr(aN)) { > + return 0; > + } else if (aN > aRange.mIdeal.value()) { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review206634 r=me with noted changes. ::: dom/media/webrtc/MediaEngineCameraVideoSource.h:27 (Diff revisions 3 - 4) > +// Fitness distance is defined in > +// https://www.w3.org/TR/2017/CR-mediacapture-streams-20171003/#dfn-selectsettings > +// The main difference of feasibility and fitness distance is that if the > +// constraint is required ('max', or 'exact'), and the settings dictionary's value > +// for the constraint does not satisfy the constraint, the fitness distance is > +// positive infinity. Please note that given a continuous space of settings dictionaries comprising all discrete combinations of dimension and frame-rate related properties, that the feasibility distance is still in keeping with the constraints algorithm. ::: dom/media/webrtc/MediaTrackConstraints.h:87 (Diff revisions 3 - 4) > void Intersect(const Range& aOther) { > - MOZ_ASSERT(Intersects(aOther)); > mMin = std::max(mMin, aOther.mMin); > + if (Intersects(aOther)) { > - mMax = std::min(mMax, aOther.mMax); > + mMax = std::min(mMax, aOther.mMax); > + } else { > + // If there is no intersection, we will use down-scaling & frame-dropping > + mMax = std::max(mMax, aOther.mMax); Range::Intersect() is a generic range intersection function. It is even used to resolve boolean constraints like echoCancellation! [1] The assert is there because the contract is you're always supposed to do: if (a.Intersects(b)) { a.Intersect(b); } Overloading a generic function with special behavior should be the last resort. Can you show where your patch requires this new behavior, and can we move this exceptional logic there instead? If not, please add a comment explaining why, and where this is used. [1] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/dom/media/webrtc/MediaTrackConstraints.h#168 ::: dom/media/webrtc/MediaTrackConstraints.cpp:435 (Diff revisions 3 - 4) > return 0; > } else if (aN > aRange.mIdeal.value()) { > return uint32_t(ValueType((std::abs(aN - aRange.mIdeal.value()) * 1000) / > 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) / > + return (UINT32_MAX / 4) + uint32_t(ValueType((std::abs(aN - aRange.mIdeal.value()) * 1000) / (UINT32_MAX / 4) is still too high a number IMHO, since fitness distances are additive. I'd be comfortable using 10000 or less. That's still 10 times stronger than a missed boolean constraint (like deviceId)).

Comment on attachment 8904985 [details] Bug 1388219 - down scale camera output frame to the target capability. https://reviewboard.mozilla.org/r/176828/#review206664 Lgtm. Note, first comment is a bitrot-review. ::: dom/media/systemservices/CamerasParent.cpp:55 (Diff revisions 6 - 7) > - return (UINT32_MAX / 2) + (requested - candidate) * > + distance = (UINT32_MAX / 2) + (requested - candidate) * > 1000 / std::max(candidate, requested); I realize this already landed, but (UINT32_MAX / 2) is too high a number IMHO, since fitness distances are additive. I'd be comfortable using 10000 or less. That's still 10 times stronger than a missed boolean constraint (like deviceId)). ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:545 (Diff revision 7) > + mMonitor.Unlock(); > + mBufMonitor.Lock(); > #ifdef DEBUG > - static uint32_t frame_num = 0; > + static uint32_t frame_num = 0; > - LOGFRAME(("frame %d (%dx%d); timeStamp %u, ntpTimeMs %" PRIu64 ", renderTimeMs %" PRIu64, > + LOGFRAME(("frame %d (%dx%d); timeStamp %u, ntpTimeMs %" PRIu64 ", renderTimeMs %" PRIu64, > - frame_num++, mWidth, mHeight, > + frame_num++, mWidth, mHeight, > - aProps.timeStamp(), aProps.ntpTimeMs(), aProps.renderTimeMs())); > + aProps.timeStamp(), aProps.ntpTimeMs(), aProps.renderTimeMs())); > #endif > > - // implicitly releases last image > + // implicitly releases last image > - mImage = image.forget(); > + mImages[i] = image.forget(); > + mBufMonitor.Unlock(); > + mMonitor.Lock(); Nit: this lock swapping is ugly. Can we solve it with scopes and MonitorAutoLock somehow?

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review206634 > Please note that given a continuous space of settings dictionaries comprising all discrete combinations of dimension and frame-rate related properties, that the feasibility distance is still in keeping with the constraints algorithm. Sorry, I don't quite understand this comment. Could you elaborate more?

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review186272 > This merge constructor resolves inherently conflicting constraints by returning an error. E.g. > > {width: {max: 640}} and {width: {min: 1024}} // OverconstrainedError > > but with downscaling, these are no longer conflicts. > > It might be time to abandon the UpdateSingleSource abstraction (for video) here. Please check my comment below.

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review206634 > Range::Intersect() is a generic range intersection function. It is even used to resolve boolean constraints like echoCancellation! [1] > > The assert is there because the contract is you're always supposed to do: > > if (a.Intersects(b)) { > a.Intersect(b); > } > > Overloading a generic function with special behavior should be the last resort. Can you show where your patch requires this new behavior, and can we move this exceptional logic there instead? If not, please add a comment explaining why, and where this is used. > > [1] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/dom/media/webrtc/MediaTrackConstraints.h#168 This change is to address above comment. Previously, Range::Merge() will return false if Intersects() return 0 (no intersection). Since now we support downscaling, Range::Merge() should always return true. However, since it is a generic function, I will try to overload it for resolution case.

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review206634 > This change is to address above comment. > Previously, Range::Merge() will return false if Intersects() return 0 (no intersection). > Since now we support downscaling, Range::Merge() should always return true. > However, since it is a generic function, I will try to overload it for resolution case. The place we merge width & height * frameRate should be here. https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/dom/media/webrtc/MediaTrackConstraints.cpp#345-355 I will modify this part.

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review206634 > Sorry, I don't quite understand this comment. Could you elaborate more? Ah sorry, I meant please add the following text to this comment e.g.: "Given a continuous space of settings dictionaries comprising all discrete combinations of dimension and frame-rate related properties, the feasibility distance is still in keeping with the constraints algorithm." Some words that say we still follow the constraints algorithm in practice.

Comment on attachment 8904984 [details] Bug 1388219 - add a nsTArray mTargetCapability to record each track target capability. https://reviewboard.mozilla.org/r/176826/#review206634 > The place we merge width & height * frameRate should be here. > https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/dom/media/webrtc/MediaTrackConstraints.cpp#345-355 > > I will modify this part. Great, because there are still cases where we need this to work the old way. E.g. {echoCancellation: {exact: true}} should throw OverconstrainedError on an audio track if another audio track in the same document has already set {echoCancellation: {exact: false}} (because we're limited to using the same MSG within the same document).

Comment on attachment 8904985 [details] Bug 1388219 - down scale camera output frame to the target capability. https://reviewboard.mozilla.org/r/176828/#review206664 > Nit: this lock swapping is ugly. Can we solve it with scopes and MonitorAutoLock somehow? I tried to use scope & MonitorAutoLock at first, but then I found it is difficult because the snippet accessing mImages is in a for loop.

Comment on attachment 8904985 [details] Bug 1388219 - down scale camera output frame to the target capability. https://reviewboard.mozilla.org/r/176828/#review206664 > I tried to use scope & MonitorAutoLock at first, but then I found it is difficult because the snippet accessing mImages is in a for loop. Wouldn't this be functionally equivalent? ``` { MonitorAutoUnlock(mMonitor); MonitorAutoLock(mBufMonitor); (...) mImages[i] = image.forget(); } ``` Since this is at the end of the loop's scope you don't even need to add an extra scope, but it could help with clarity.

Thanks for the reminding! I will replace the snippet with this.

Pushed by mchiang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/341aaeb4ce69 add a nsTArray mTargetCapability to record each track target capability. r=jib https://hg.mozilla.org/integration/autoland/rev/213c2c200c08 down scale camera output frame to the target capability. r=jib

This broke mingw32 Windows builds. https://treeherder.mozilla.org/logviewer.html#?job_id=147819312&repo=autoland [task 2017-11-27T13:32:05.503Z] 13:32:05 INFO - INPUT("StaticXULComponentsEnd/StaticXULComponentsEnd.o") [task 2017-11-27T13:32:05.504Z] 13:32:05 INFO - ../../dom/media/webrtc/Unified_cpp_dom_media_webrtc0.o: In function `ZN7mozilla17MediaEngineSource16AllocationHandleC1ERKNS_3dom21MediaTrackConstraintsERKNS_3ipc13PrincipalInfoERKNS_16MediaEnginePrefsERK9nsTStringIDsE': [task 2017-11-27T13:32:05.504Z] 13:32:05 INFO - /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngine.h:231: undefined reference to `mozilla::MediaEngineSource::AllocationHandle::sId' [task 2017-11-27T13:32:05.504Z] 13:32:05 INFO - /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngine.h:231: undefined reference to `mozilla::MediaEngineSource::AllocationHandle::sId' [task 2017-11-27T13:32:05.504Z] 13:32:05 INFO - /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngine.h:231: undefined reference to `mozilla::MediaEngineSource::AllocationHandle::sId' [task 2017-11-27T13:32:05.504Z] 13:32:05 INFO - /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngine.h:231: undefined reference to `mozilla::MediaEngineSource::AllocationHandle::sId' [task 2017-11-27T13:32:05.505Z] 13:32:05 INFO - collect2: error: ld returned 1 exit status [task 2017-11-27T13:32:05.505Z] 13:32:05 INFO - gmake[4]: *** [xul.dll] Error 1 Tom, can you take a look at this, please?

This is being tracked in Bug 1421025

As explained in the new bug, this patch breaks --disable-webrtc

https://hg.mozilla.org/mozilla-central/rev/341aaeb4ce69 https://hg.mozilla.org/mozilla-central/rev/213c2c200c08

Backed out for causing bug 1421706. https://hg.mozilla.org/mozilla-central/rev/91fc3a79606b

Fix audio delay issue

Comment on attachment 8904985 [details] Bug 1388219 - down scale camera output frame to the target capability. https://reviewboard.mozilla.org/r/176828/#review210186 Lgtm.

Pushed by mchiang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3ac626fba7fa add a nsTArray mTargetCapability to record each track target capability. r=jib https://hg.mozilla.org/integration/autoland/rev/741eed8ed628 down scale camera output frame to the target capability. r=jib

https://hg.mozilla.org/mozilla-central/rev/3ac626fba7fa https://hg.mozilla.org/mozilla-central/rev/741eed8ed628

Should bug 1434353 also depend on this?

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #75) > Should bug 1434353 also depend on this? I just duped bug 1434353 to bug 1434538 which we have strong reasons to believe is the real cause. That dependency is already set up.