Closed Bug 1423582 Opened 2 years ago Closed 2 years ago

Large-buffer leak in MediaEngineRemoteVideoSource

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → mchiang
Rank: 9
Priority: -- → P1
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
Attachment #8936674 - Flags: review?(apehrson) → review-
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
Keywords: regression
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.
Flags: needinfo?(apehrson)
That would be fine.
Flags: needinfo?(apehrson)
Should someone else take this (I'm honestly not sure)?
Flags: needinfo?(jib)
I can take this.
Assignee: bonchiang → apehrson
Flags: needinfo?(jib)
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.
Attachment #8936674 - Flags: review- → review+
I'll push a new patch just so we have one based on recent m-c.
Status: NEW → ASSIGNED
Attachment #8936674 - Attachment is obsolete: true
Comment on attachment 8947402 [details]
Bug 1423582 - use UniquePtr to wrap frame.

https://reviewboard.mozilla.org/r/217122/#review222936
Attachment #8947402 - Flags: review?(apehrson) → review+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee905270a20a
use UniquePtr to wrap frame. r=pehrsons
https://hg.mozilla.org/mozilla-central/rev/ee905270a20a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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
Attachment #8947402 - Flags: approval-mozilla-beta?
Comment on attachment 8947402 [details]
Bug 1423582 - use UniquePtr to wrap frame.

Fixes a leak, looks low-risk. Taking for 59b7.
Attachment #8947402 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.