Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
912293005:
Mac Video Capture AVFoundation: Calculate compressed frame size for MJPEG. (Closed)
|
Created:
5 years, 10 months ago by magjed_chromium Modified:
5 years, 8 months ago CC:
oja_google.com, fbarchard, chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac Video Capture AVFoundation: Calculate compressed frame size for MJPEG.
The frame size we receive from AVFoundation is sometimes excessive, e.g. the size of the uncompressed frame. For example, for a JPEG YUV422, the reported size might be 1280*720*2 = 1.8MB, while the actual compressed data is only 260kB.
In these cases, libyuv::MJPGToI420 spends a lot of time in libyuv::ValidateJpeg, as much as 25%. The reason for this is that ValidateJpeg starts at the end of the buffer and loops backwards looking for the End Of Image (EOI) marker.
This CL tries to optimize these cases by finding the EOI by looping forwards instead of backwards, and reporting the actual frame size to libyuv::MJPGToI420.
BUG=346634, 460497
Committed: https://crrev.com/dc556d2b54354adc95ee5474600f469f7ae65692
Cr-Commit-Position: refs/heads/master@{#317333}
Patch Set 1 #
Total comments: 3
Patch Set 2 : todo #Messages
Total messages: 25 (5 generated)
magjed@chromium.org changed reviewers: + mcasas@chromium.org, perkj@chromium.org, tommi@chromium.org, torbjorng@google.com
Please take a look. I couldn't manage to get the correct frame size from AVFoundation. Maybe it's camera specific? Anyway, this CL should reduce the time spent searching for the EOI marker significantly for large frame sizes.
https://codereview.chromium.org/912293005/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_avfoundation_mac.mm (right): https://codereview.chromium.org/912293005/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:328: // jpeg decoding in the browser. what about adding a note about the validation step in libvpx? Since that's where we're spending cycles right now, it will help a future reader understand the context. https://codereview.chromium.org/912293005/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:330: frameSize = JpegFrameSize(baseAddress, frameSize); If JpegFrameSize fails and returns 0, should we keep the previous value?
|frame_size| is passed as an indication to VideoCaptureDeviceMac::ReceiveFrame() [1]; this one forwards it verbatim to VCController::...::OnIncomingCapturedData() [2], that again sends it verbatim to libyuv (not libvpx). You could argue, and rightly so, that the semantics of |frame_size| are misleading, since it seems to accompany the memory backed buffer and expected to be consistent with it ([3] is lying). Bottom line is that this number could perfectly be bonkers until libyuv has to deal with it. So, I'd say this class _not_ the place to deal with (M)JPEG quirks (which could appear in other platforms), although is confusing as to why not. This optimization should go to libyuv. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... [3] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
torbjorng@chromium.org changed reviewers: + torbjorng@chromium.org
LGTM. The logic looks absolutely correct; this type of code is prone to fencepost errors so I looked carefully for those. :-) Did some timing tests, and memchr indeed seems faster than sentinel based byte loops (7x) or word-by-word trickery (2x). That's because memchr is written in SIMD assembly. https://codereview.chromium.org/912293005/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_avfoundation_mac.mm (right): https://codereview.chromium.org/912293005/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:40: (it = static_cast<const char*>(memchr(it, 0xff, end - it))); I'm not too fond of side-effects in loop termination criteria, consider putting memchr code in loop body for readability. Also, while pointer-interpreted-as-boolean is OK in the language, is it really accepted by all compilers?
I agree with Miguel that this is not the right place to put MJPEG stuff, and that libyuv would be more suitable. Actually, it would be optimal to fix chromium_jpeg_read_header so that we can rely on it never crashing for malformed jpeg data. However, that will take some time, and this is more of a quick fix. Miguel, would you be satisfied with a TODO? :) If memchr is 7x times faster than a manual for loop, maybe it's sufficient to to replace the for loop in libyuv::ValidateJpeg with memrchr (reversed memchr)?
My measurement compared Mac OS libc's memchr to a well-tweaked sentinel based byte-for-byte loop. That was a 7x difference to memchr's advantage. Incidentally, the difference on Linux is greater. I think the authors of glibc memchr must be very clever. :-) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/12 15:35:28, magjed wrote: > I agree with Miguel that this is not the right place to put MJPEG stuff, and > that libyuv would be more suitable. Actually, it would be optimal to fix > chromium_jpeg_read_header so that we can rely on it never crashing for malformed > jpeg data. > > However, that will take some time, and this is more of a quick fix. Miguel, > would you be satisfied with a TODO? :) > Nay sorry. This is the kind of thing we'd like to have in all platforms as well, right? Good news is that libyuv reviews go very fast. Ping fbarchard@ and he might do it on the spot, then wait for the roll and reap the benefits :) As a side note, what about launching torbjorng@ against libyuv code? :) https://code.google.com/p/libyuv/source/browse/#svn%2Ftrunk%2Fsource > If memchr is 7x times faster than a manual for loop, maybe it's sufficient to to > replace the for loop in libyuv::ValidateJpeg with memrchr (reversed memchr)? Just throwing some more noise, I read in the man page about the alternative rawmemchr(), quoting: The rawmemchr() function is similar to memchr(): it assumes (i.e., the programmer knows for certain) that an instance of c lies somewhere in the memory area starting at the location pointed to by s, and so performs an optimized search for c (i.e., no use of a count argument to limit the range of the search). Can we use it in some clever way?
I don't think rawmemchr is portable. I love sentinel techniques for this sort of problems. I.e., insert the search string at the end of the buffer, and thereby avoid the overhead for loop bookkeeping. A sentinel is required for rawmemchr in the issue at hand. For a tight loop, loop bookkeeping can easily be 50% of the cost. But when using parallel search as do libc memchr and memrchr, sentinel techniques tend be be less important; we pretty much run at cache speed anyway. TG To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
New patchsets have been uploaded after l-g-t-m from torbjorng@chromium.org
This is fixed in latest libyuv already, but the last roll failed. I'm committing this to get the performance improvement into the cut. I will remove this once we have successfully rolled libyuv.
On 2015/02/20 15:51:17, magjed wrote: > This is fixed in latest libyuv already, but the last roll failed. I'm committing > this to get the performance improvement into the cut. I will remove this once we > have successfully rolled libyuv. lgtm. Once libyuv has been rolled we'll reconvene.
The CQ bit was checked by magjed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/912293005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/dc556d2b54354adc95ee5474600f469f7ae65692 Cr-Commit-Position: refs/heads/master@{#317333}
Message was sent while issue was closed.
On 2015/02/20 16:30:16, I haz the power (commit-bot) wrote: > Patchset 2 (id:??) landed as > https://crrev.com/dc556d2b54354adc95ee5474600f469f7ae65692 > Cr-Commit-Position: refs/heads/master@{#317333} Ehm... What's the bug number to remove this extra code when "Chromium has the latest libyuv version"?
Message was sent while issue was closed.
On 2015/02/20 16:36:41, mcasas wrote: > On 2015/02/20 16:30:16, I haz the power (commit-bot) wrote: > > Patchset 2 (id:??) landed as > > https://crrev.com/dc556d2b54354adc95ee5474600f469f7ae65692 > > Cr-Commit-Position: refs/heads/master@{#317333} > > Ehm... > What's the bug number to remove this extra code when > "Chromium has the latest libyuv version"? I'll file one right now and let me see if I can't add you to the discussion threads with Frank and the other guys (if not, Magnus can you do it?). The basic story is that Frank wrote a patch that improves the validation check by about a factor of 10x and we tried doing a roll of libyuv with that fix, today. Unfortunately it's been several months since libyuv was last rolled, so the roll includes a lot of unrelated changes and indeed we ran into crashes. Doing such a big roll today of all days, is also not a good idea. So we rolled it back. However, given that we spend a significant time today validating frames from AVFoundation, and that the reason for having a validation step in the first place is questionable (and arguably not correct either), I figured we should work around the performance problem in a low risk way and save a significant amount of CPU. For M43 we can figure out what the issues are with libyuv and have a much longer bake time for all the cls that have yet to be rolled into chrome.
Message was sent while issue was closed.
On 2015/02/20 16:36:41, mcasas wrote: > On 2015/02/20 16:30:16, I haz the power (commit-bot) wrote: > > Patchset 2 (id:??) landed as > > https://crrev.com/dc556d2b54354adc95ee5474600f469f7ae65692 > > Cr-Commit-Position: refs/heads/master@{#317333} > > Ehm... > What's the bug number to remove this extra code when > "Chromium has the latest libyuv version"? I filed a new bug here: crbug/460497
Message was sent while issue was closed.
On 2015/02/20 16:56:13, tommi wrote: > On 2015/02/20 16:36:41, mcasas wrote: > > On 2015/02/20 16:30:16, I haz the power (commit-bot) wrote: > > > Patchset 2 (id:??) landed as > > > https://crrev.com/dc556d2b54354adc95ee5474600f469f7ae65692 > > > Cr-Commit-Position: refs/heads/master@{#317333} > > > > Ehm... > > What's the bug number to remove this extra code when > > "Chromium has the latest libyuv version"? > > I'll file one right now and let me see if I can't add you to the discussion > threads with Frank and the other guys (if not, Magnus can you do it?). > The basic story is that Frank wrote a patch that improves the validation check > by about a factor of 10x and we tried doing a roll of libyuv with that fix, > today. Unfortunately it's been several months since libyuv was last rolled, so > the roll includes a lot of unrelated changes and indeed we ran into crashes. > Doing such a big roll today of all days, is also not a good idea. So we rolled > it back. However, given that we spend a significant time today validating > frames from AVFoundation, and that the reason for having a validation step in > the first place is questionable (and arguably not correct either), I figured we > should work around the performance problem in a low risk way and save a > significant amount of CPU. > For M43 we can figure out what the issues are with libyuv and have a much longer > bake time for all the cls that have yet to be rolled into chrome. Filed: crbug.com/460498
Message was sent while issue was closed.
On 2015/02/20 16:58:22, tommi wrote: > On 2015/02/20 16:56:13, tommi wrote: > > On 2015/02/20 16:36:41, mcasas wrote: > > > On 2015/02/20 16:30:16, I haz the power (commit-bot) wrote: > > > > Patchset 2 (id:??) landed as > > > > https://crrev.com/dc556d2b54354adc95ee5474600f469f7ae65692 > > > > Cr-Commit-Position: refs/heads/master@{#317333} > > > > > > Ehm... > > > What's the bug number to remove this extra code when > > > "Chromium has the latest libyuv version"? > > > > I'll file one right now and let me see if I can't add you to the discussion > > threads with Frank and the other guys (if not, Magnus can you do it?). > > The basic story is that Frank wrote a patch that improves the validation check > > by about a factor of 10x and we tried doing a roll of libyuv with that fix, > > today. Unfortunately it's been several months since libyuv was last rolled, > so > > the roll includes a lot of unrelated changes and indeed we ran into crashes. > > Doing such a big roll today of all days, is also not a good idea. So we > rolled > > it back. However, given that we spend a significant time today validating > > frames from AVFoundation, and that the reason for having a validation step in > > the first place is questionable (and arguably not correct either), I figured > we > > should work around the performance problem in a low risk way and save a > > significant amount of CPU. > > For M43 we can figure out what the issues are with libyuv and have a much > longer > > bake time for all the cls that have yet to be rolled into chrome. > > Filed: crbug.com/460498 hah, I've marked mine as a duplicate then.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1089173003/ by magjed@chromium.org. The reason for reverting is: An improved version of ValidateJpeg in libyuv has been rolled into chromium and this CL is no longer needed..
Message was sent while issue was closed.
fbarchard@chromium.org changed reviewers: + fbarchard@chromium.org
Message was sent while issue was closed.
lgtm would still be good to find a way to determine the correct size of the frame. A todo perhaps. |
