Open
Bug 984215
Opened 11 years ago
Updated 2 years ago
VideoConduit reads the rotation flag and decides the rotation degree
Categories
(Core :: WebRTC: Audio/Video, defect, P5)
Tracking
()
NEW
Future
backlog | webrtc/webaudio+ |
People
(Reporter: slee, Unassigned, NeedInfo)
References
Details
(Whiteboard: [WebRTC][ft:multimedia-platform])
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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 2•11 years ago
|
||
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.
Reporter | ||
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
(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).
Reporter | ||
Comment 5•11 years ago
|
||
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.
Reporter | ||
Comment 6•11 years ago
|
||
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•11 years ago
|
||
(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).
Reporter | ||
Comment 8•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [WebRTC] → [WebRTC][ft:multimedia-platform]
Target Milestone: --- → 2.0 S1 (9may)
Comment 10•11 years ago
|
||
Hi Alfredo, Please help to handle on this, thanks a lot.
Assignee: rlin → ayang
Comment 12•11 years ago
|
||
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?
Flags: needinfo?(ayang)
Comment 13•11 years ago
|
||
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.
Flags: needinfo?(ayang)
Updated•10 years ago
|
Target Milestone: 2.0 S1 (9may) → Future
Comment 14•9 years ago
|
||
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.
backlog: --- → webRTC+
Rank: 42
Priority: -- → P4
Comment 15•7 years ago
|
||
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
Comment 16•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 17•3 years ago
|
||
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.
Assignee: ayang → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jib)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•