The allocation at [1] is infallible, but surrounding code suggests that it was meant to be fallible (the `if (!frame)` check). We should then change it to be fallible, see [2], but then we risk leaking this allocation at [3]. We should never use raw pointers like this. We can wrap them in `UniquePtr` instead. [1] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#511 [2] https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation#Explicitly_fallible_memory_allocators [3] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#544

Munro, this is your code so I'm assigning you. P1 but low prio since it hasn't made it past Nightly yet.

If I wrap frame in UniquePtr, it will crash at here. https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#560

Comment on attachment 8936674 [details] Bug 1423582 - use UniquePtr to wrap frame. https://reviewboard.mozilla.org/r/207412/#review213354 ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:514 (Diff revision 2) > - frame = new unsigned char[properties.bufferSize()]; > + frameBuf.reset(new (fallible) uint8_t[properties.bufferSize()]); > + frame = frameBuf.get();; Good. This is an improvement and solves the leak. However, while you're here touching this code, and since we don't need to do uplifts, let's optimize this a bit. Currently when we have to rescale, we: - Create a `webrtc::I420Buffer` at the original resolution, and copy the original rawptr image buffer into that. (I'm not clear on why. Seemingly to convert to I420, but we don't do that if we don't have to rescale) - Create a `webrtc::I420Buffer` at the scaled resolution and scale the originally-sized `I420Buffer` into that with util methods of `I420Buffer`. - Create a raw buffer (the one we risk leaking here) at the scaled resolution and copy the scaled `I420Buffer` into that with our own `VideoFrameUtils` library. - Then we create a buffer by creating a `layers::PlanarYCbCrImage` instance, which can be fed to our MediaStream pipeline. We copy into that with our graphics APIs. Let's cut these four copies to one. I suggest we do what `CropAndScaleFrom` does and let libyuv crop and scale straight into the buffer of our `PlanarYCbCrImage`, see [1]. This also has the benefit of using a buffer recycler, which the raw buffer allocations don't use. That alone might help perf more than the actual copying (though currently there's a lot of copying so maybe not). [1] https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/media/webrtc/trunk/webrtc/api/video/i420_buffer.cc#231

Thanks for the review. I will do the optimization.

Note: we should probably also check that the buffer recycler works as expected so we are indeed not allocating there buffer recycler is a part of ImageContainer

Could we separate rescaling optmization (reduce memory copy) to another bug? This bug is marked as a regression and tracked by regression-triage. I am occupied by other stability bugs and may start to address this later this week.

That would be fine.

Should someone else take this (I'm honestly not sure)?

I can take this.

Comment on attachment 8936674 [details] Bug 1423582 - use UniquePtr to wrap frame. https://reviewboard.mozilla.org/r/207412/#review222930 r+ with fixing the copying in followup bug 1434861.

I'll push a new patch just so we have one based on recent m-c.

Comment on attachment 8947402 [details] Bug 1423582 - use UniquePtr to wrap frame. https://reviewboard.mozilla.org/r/217122/#review222936

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ee905270a20a use UniquePtr to wrap frame. r=pehrsons

Comment on attachment 8947402 [details] Bug 1423582 - use UniquePtr to wrap frame. Approval Request Comment [Feature/Bug causing the regression]: bug 1388219 [User impact if declined]: Increased risk of OOM (in a fairly narrow case we risk leaking large memory buffers, we also increase the risk intermittently because we do infallible allocations of large buffers) [Is this code covered by automated tests?]: No (there's an assert in debug and we haven't seen failures, I doubt it would get hit in release) [Has the fix been verified in Nightly?]: No, this is hard to test. Only triggered when we are already low on available memory. We're trying to be proactive with this fix. [Needs manual test from QE? If yes, steps to reproduce]: No. Again hard to test and reliably reproduce. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Simple and safe patch that makes sure a buffer doesn't outlive it's scope. There's no usage of it outside the scope. [String changes made/needed]: None

Comment on attachment 8947402 [details] Bug 1423582 - use UniquePtr to wrap frame. Fixes a leak, looks low-risk. Taking for 59b7.