1206708 - WebRTC video is doing copying video frames more than it needs to

Status: RESOLVED FIXED

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

Description

[Nicolas Silva [:nical]]

:padenot was looking at some profiles to understand the high cpu usage when using hello on mac and noticed that we go throught TextureClient::UpdateYCbCr on the ImageBridge thread. This means that the Image object that is used with ImageContainer::SetCurrentImage is not implemented on top of TextureClient, and that when comes the time to send the video frame to the compositor, we go through the slow path for each frame: we allocate a new texture and copy the image into it both those things are pretty slow.
We can do better by using an Image implementation that is implemented on top of TextureClient so that it can be sent to the compositor without an extra allocation + copy (for instance SharedPlanarYCbCrImage).

Comment 1

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

Sounds great - is there a pointer to some code that has an implementation of this?

Comment 2

[Nicolas Silva [:nical]]

(In reply to Randell Jesup [:jesup] from comment #1)
> Sounds great - is there a pointer to some code that has an implementation of
> this?

So I don't know what Image class is being used with webrtc on mac, but the way the "classic" video pipeline selects the proper image type is by asking the ImageContainer to create it (ImageContainer::CreateImage calls ImageClient::CreateImage, which creates the Shared<Blah>Image class depending on the image format. for instance here: https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/dom/media/MediaData.cpp#321

So I would look at where the Image object is created, whether it uses ImageContainer::CreateImage or if it does its own thing. If it does the former, it's probably that ImageContainer::CreateImage falls back to a non-shared version of the Image.

If things work in the end, you should not take this branch anymore: https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/gfx/layers/client/ImageClient.cpp#174

It's possible that there are some other less obvious prerequisites that failed and we end up in the slow path in ImageClient.cpp (in this case the starting point of the investigation would be that branch), but it's most likely that we don't create the right Image type.

Comment 3

[Nicolas Silva [:nical]]

summary from teh quick discussion in Orlando:

Places where copying yuv frames occur:
* MappedYCbCrTextureData::CopyInto
* YCbCrImageDataSerializer::CopyData

Look into RecyclingPlanarYCbCrImage. Usually, if we use a SharedPlanarYCbCrImage instead, it'll avoid a copy.

Comment 4

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

We should also try to integrate this with the "native" image support in webrtc.org videoframes, which is designed to minimize copying and keep stuff in gfxmem as much as possible (and leverage that when HW encoders or decoders are in use)

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1206708 - Make all WebRTC ImageContainers async. r?nical,jesup

This avoids expensive copying when compositing these frames on e10s.

Review commit: https://reviewboard.mozilla.org/r/45465/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45465/

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1206708 - Allow YUV images with gaps between planes in MediaPipelineTransmit. r?jesup

Review commit: https://reviewboard.mozilla.org/r/46399/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46399/

Comment 7

[Andreas Pehrson [:pehrsons]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da5e30ca94c6

Mostly good. Though this triggers a known shutdown issue that we'll need to resolve before landing.

With Nightly, when profiling on talky.io talking to another machine-local (other process) peer, this patch save us 7% of the samples spent doing work. That is, the thread sending frames to the compositor uses 6.5% of all samples, compared to 13.5% when it has to copy the frames.

Other copies happening to/from webrtc.org code are still occurring like before. We should file another bug for integrating the frame types.

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 8741341 [details]
MozReview Request: Bug 1206708 - Make all WebRTC ImageContainers async. r?nical,jesup

https://reviewboard.mozilla.org/r/45465/#review42975

Comment 9

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

Comment on attachment 8741341 [details]
MozReview Request: Bug 1206708 - Make all WebRTC ImageContainers async. r?nical,jesup

https://reviewboard.mozilla.org/r/45465/#review43161

Comment 10

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

Comment on attachment 8741342 [details]
MozReview Request: Bug 1206708 - Allow YUV images with gaps between planes in MediaPipelineTransmit. r?jesup

https://reviewboard.mozilla.org/r/46399/#review43159

r+ if we can add a debug/assertion back (unless there's a good reason not to)

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:366
(Diff revision 1)
> +        MOZ_MTLOG(ML_DEBUG, "Sending an I420 video frame");
> +        VideoFrameConverted(i420_frame);
> +        return;
>        }

It appears we lost any sort of debug/assertion on not getting ycrcb data....

Comment 11

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/46399/#review43159

> It appears we lost any sort of debug/assertion on not getting ycrcb data....

The old check was for contiguous planes, but the modified code can live without that. There's not really a way to fail this anymore -- so what do you want to assert?

Comment 12

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

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #11)
> https://reviewboard.mozilla.org/r/46399/#review43159
> 
> > It appears we lost any sort of debug/assertion on not getting ycrcb data....
> 
> The old check was for contiguous planes, but the modified code can live
> without that. There's not really a way to fail this anymore -- so what do
> you want to assert?

You're right; misread it - thought it was for the if (data) test.  Never mind - land!

Comment 13

[Andreas Pehrson [:pehrsons]]

Final check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d476e89b664

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08f50e080cdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/13f36f3e3181

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08f50e080cdd
https://hg.mozilla.org/mozilla-central/rev/13f36f3e3181