984215 - VideoConduit reads the rotation flag and decides the rotation degree

Status: NEW

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

Description

[StevenLee[:slee]]

As bug 980266 mentioned, bug 976397 fixes the rotation degree of local view. This bug will retrieve the correct degree and rotate the corresponding video frames.

Comment 1

[StevenLee[:slee]]

Attachment: Read rotation angle from GrallocImage and rotate the video frame

Comment 2

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

Note: If a gUM video feed is being used for PeerConnection, then we're going to end up rotating it in the CPU anyways with this patch, so there's a small gain at most combining this patch and the dependency to not rotate in gUM (and possibly no or negative gain in total).  If the libyuv calls to rotate-and-scale video for encoding also leveraged the GPU, there could be a win overall.

If you take gUM and feed it directly to a video element (or mostly do that and only occasionally grab real data), then you'll have a perf win.

Comment 3

[StevenLee[:slee]]

(In reply to Randell Jesup [:jesup] from comment #2)
> Note: If a gUM video feed is being used for PeerConnection, then we're going
> to end up rotating it in the CPU anyways with this patch, so there's a small
> gain at most combining this patch and the dependency to not rotate in gUM
> (and possibly no or negative gain in total).  If the libyuv calls to
> rotate-and-scale video for encoding also leveraged the GPU, there could be a
> win overall.
The problem of rotating in gUM is that, 1) not everyone prefers the rotated images, ex, MediaRecorder may have problem if the input size keeps changing. 2) performance is bad. Because we need to copy the memory from GrallocImage into system memory.

If libyuv can use GPU that should be the best way, but it seems not support using GPU, is it right? We may also implement a shader to rotate the image. I am not sure if it's performance is good, too.

> 
> If you take gUM and feed it directly to a video element (or mostly do that
> and only occasionally grab real data), then you'll have a perf win.
Do you mean in PeerConnection we get the video data from video element directly?

Comment 4

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

(In reply to StevenLee[:slee] from comment #3)
> (In reply to Randell Jesup [:jesup] from comment #2)
> > Note: If a gUM video feed is being used for PeerConnection, then we're going
> > to end up rotating it in the CPU anyways with this patch, so there's a small
> > gain at most combining this patch and the dependency to not rotate in gUM
> > (and possibly no or negative gain in total).  If the libyuv calls to
> > rotate-and-scale video for encoding also leveraged the GPU, there could be a
> > win overall.
> The problem of rotating in gUM is that, 1) not everyone prefers the rotated
> images, ex, MediaRecorder may have problem if the input size keeps changing.

MediaRecorder needs to learn to adjust to rotations in any case.  If you're recording in portrait and the camera is in landscape, it needs to rotate before encoding (just like PeerConnection).  The possible difference would be if the output format can't handle mid-record changes in size, in which case either it has to "lock" rotation at start time (by either telling gUM to lock (not supported, not standard) or by counter-rotating to compensate for changes, or it has to scale on during-recording rotations.  I.e. recording in landscape, no rotation, 640x480.  User rotates to Portrait; gUM changes to 480x640.  MediaRecorder starts scaling from 480x640 to 360x480 inset with black bars on left and right to 640x480.

> 2) performance is bad. Because we need to copy the memory from GrallocImage
> into system memory.

For PeerConnection we need to anyways; though perhaps with changes we could replace a libyuv scale-and-rotate-and-convert-to-4:2:0 with a GPU scale-and-rotate (perhaps with SW 4:2:0 conversion).

> If libyuv can use GPU that should be the best way, but it seems not support
> using GPU, is it right? We may also implement a shader to rotate the image.
> I am not sure if it's performance is good, too.

libyuv does not, though we could add some code to pre-rotate/scale with the GPU in VideoConduit before feeding it to the frame input code.

> > If you take gUM and feed it directly to a video element (or mostly do that
> > and only occasionally grab real data), then you'll have a perf win.
> Do you mean in PeerConnection we get the video data from video element
> directly?

No, I mean the performance win you list with the current "avoid rotation in GUM" only helps if we assign the MediaStream directly to a <video> element and do nothing else with it.  In all other cases, we must get access to a rotated-in-CPU-memory copy at some point (perhaps combined with a scale or 4:2:0 conversion).

Comment 5

[StevenLee[:slee]]

Thanks for the clarification. I misunderstood some of your reply.

(In reply to Randell Jesup [:jesup] from comment #4)
> for changes, or it has to scale on during-recording rotations.  I.e.
> recording in landscape, no rotation, 640x480.  User rotates to Portrait; gUM
> changes to 480x640.  MediaRecorder starts scaling from 480x640 to 360x480
> inset with black bars on left and right to 640x480.
Yes, that's bug 984216 is going to do.

> For PeerConnection we need to anyways; though perhaps with changes we could
> replace a libyuv scale-and-rotate-and-convert-to-4:2:0 with a GPU
> scale-and-rotate (perhaps with SW 4:2:0 conversion).
I will have a simple test about the performance differences between scale, rotate, and covert. Then we can know which one is the best candidate to run on GPU.

> No, I mean the performance win you list with the current "avoid rotation in
> GUM" only helps if we assign the MediaStream directly to a <video> element
> and do nothing else with it.  In all other cases, we must get access to a
> rotated-in-CPU-memory copy at some point (perhaps combined with a scale or
> 4:2:0 conversion).
With this patch, we can eliminate some memory copy. Before applying the patch, the flow is, 
1. camera callback, color space conversion and rotation
2. MSG callback to display
3. MediaPipeline::ProcessVideoFrame

1, 2, and 3 have memory copy.

But after applying the patch, 1 only set rotation flag. In 2, compositor has better performance when handling GrallocImage. 3 still needs memory copy. So that I think the performance should be better then the original version.

Comment 6

[StevenLee[:slee]]

Here is some test results on unagi. The source size is always 640x480. It shows that CPU usage order, rotate > scale > conversion.

* convert from NV21 to I420
** fps: 541.712
* rotate 90
** fps: 256.739
* scale 640x480 -> 480x360
** kFilterNone, fps: 799.361
** kFilterLinear, fps: 472.367
** kFilterLBilinear, fps: 334.448
** kFilterBox(default in GIPS), fps: 332.889

Comment 7

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

(In reply to StevenLee[:slee] from comment #5)

> > No, I mean the performance win you list with the current "avoid rotation in
> > GUM" only helps if we assign the MediaStream directly to a <video> element
> > and do nothing else with it.  In all other cases, we must get access to a
> > rotated-in-CPU-memory copy at some point (perhaps combined with a scale or
> > 4:2:0 conversion).
> With this patch, we can eliminate some memory copy. Before applying the
> patch, the flow is, 
> 1. camera callback, color space conversion and rotation
> 2. MSG callback to display
> 3. MediaPipeline::ProcessVideoFrame
> 
> 1, 2, and 3 have memory copy.
> 
> But after applying the patch, 1 only set rotation flag. In 2, compositor has
> better performance when handling GrallocImage. 3 still needs memory copy. So
> that I think the performance should be better then the original version.

Right - if you also show a local copy, the compositor likely has a win, but you still need the rotated version in CPU memory for MediaPipeline.  And many video chat apps show a (small) local image, though on mobile it's likely to be very small if it's there at all.  So, for the case above, we have to rotate-and-convert for MediaPipeline, and we also have to rotate in the GPU for the compositor.  So we're doing it twice, but the second one may be low total overhead, especially if it's also scaled to a thumbnail.

I'm just indicating it's a complex comparison.  And if we can use the GPU in MediaPipeline/VideoConduit to rotate and scale, there may be a further win (at the cost of more complexity).

Comment 8

[StevenLee[:slee]]

(In reply to Randell Jesup [:jesup] from comment #7)
> I'm just indicating it's a complex comparison.  And if we can use the GPU in
> MediaPipeline/VideoConduit to rotate and scale, there may be a further win
> (at the cost of more complexity).
OK, I got it. ctai in TPE office is trying to make GonkNativeWindow be able to process by OpenGL. When his WIP is done, I can do some tests with this patch.

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Talk to Steven and I will fix this issue.

Comment 10

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Alfredo, 
Please help to handle on this, thanks a lot.

Comment 11

[Alfredo Yang (:alfredo)]

Attachment: webrtc_rotate

Base on Steven's patch.

Comment 12

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Alfredo, 
Could you get the cpu usage deviation for the 
1. Avoid rotate in MediaEngineWebRTCVideo module (via Bug 976397)
2. Avoid the twice memory copy on webRTC module for rotation.
3. Combine 1+2
on flame device?

Comment 13

[Alfredo Yang (:alfredo)]

User 43%, System 26%, IOW 0%, IRQ 0%
User 175 + Nice 113 + Sys 174 + Idle 198 + IOW 0 + IRQ 0 + SIRQ 0 = 660

  PID   TID PR CPU% S     VSS     RSS PCY UID      Thread          Proc
 9714  9714  1   8% S 186716K  77212K     root     b2g             /system/b2g/b2g
 9714  9743  1   7% S 186716K  77212K     root     Compositor      /system/b2g/b2g
 9951  9951  0   6% S 126668K  45716K     u0_a9951 Browser         /system/b2g/plugin-container
 9714  9721  1   3% S 186716K  77212K     root     Gecko_IOThread  /system/b2g/b2g
 9951  9954  0   3% S 126668K  45716K     u0_a9951 Socket Thread   /system/b2g/plugin-container
 9951 10538  1   3% S 126668K  45716K     u0_a9951 MediaStreamGrph /system/b2g/plugin-container
 9951  9952  0   2% S 126668K  45716K     u0_a9951 Chrome_ChildThr /system/b2g/plugin-container
 9714  9722  1   2% S 186716K  77212K     root     Socket Thread   /system/b2g/b2g
 9951 11235  1   2% S 126668K  45716K     u0_a9951 ViECaptureThrea /system/b2g/plugin-container
  913   913  1   2% S      0K      0K     root     TX_Thread
 9951 11234  0   2% S 126668K  45716K     u0_a9951 ProcessThread   /system/b2g/plugin-container
  282 11246  1   1% S  69644K  14056K  fg media    VideoEncMsgThre /system/bin/mediaserver
11304 11304  0   1% R   1344K    672K     root     top             top
  282   587  0   1% S  69644K  14056K     media    AudioOut_2      /system/bin/mediaserver



This is video only call with WIP. (H264 encode only)

3% in MediaStreamGrph should be memory copy ConvertToI420 in VideoCaptureImpl::IncomingFrame().
2% in ViECaptureThrea should be memory copy to IOMX.
6% in main thread which I don't know what's going on. I will set up profiler to survey it.

Comment 14

[Maire Reavy [:mreavy]]

Would also help Android.

Also, WebRTC is moving to avoid all rotations on the sending side, and instead pass a header extension flag to indicate the rotation.  Note that this means that *other* things that are fed data by MediaStreamGraph will need to handle the rotation (or have the graph rotate for them).  Perhaps if they need rotated video they'd flag that to MSG somehow, or provide a "RotateToCorrectOrientation()" method.

Comment 15

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.

Comment 16

[Guang-De Lin]

Hello Mozilla team,

My team is working on a new project based on WebRTC, and our mobile app utilizes WebRTC prebuilt libraries (Android and iOS). When the mobile app sends video frames to Chrome browser, Chrome handles the header extension flag "video-orientation" perfectly. But it seems like Firefox does not handle the flag and the video display will be not rotated. Tested on Firefox 59.0.2 (64bit), macOS 10.13.4.

Don't know if this is a bug or a feature request. Will it be fixed or implemented in the near future?

Sorry for my disturbance if this bug is not related to my issue.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jib, could you have a look please?
For more information, please visit auto_nag documentation.