Potential video capture crash in accelerated jpeg decode on ChromeOS |
|||||
Issue descriptionI just discovered a bug that was introduced in a CL [1] that I checked in about 23 hours ago. I discovered it through a new integration test I am about to add. The symptom in production is likely going to be potential crash on ChromeOS when doing video capture with accelerated MJPEG decoding. I call it "potential crash", because the conditions that trigger it may not always occur in practice. I have a fix ready and will post it for review momentarily. In the meantime, please be aware of this issue when using a ChromeOS build from yesterday evening or newer for video capture. [1] https://chromium.googlesource.com/chromium/src/+/e09db164377ade3a5559dd9b1fae1f47756894b0
,
Feb 25 2017
The code review is up: https://codereview.chromium.org/2715713006
,
Feb 25 2017
Thank you for the quick fix. crrev.com/e09db164377ade3a5559dd9b1fae1f47756894b0 landed in 58.0.3022.0. CrOS is still on 58.0.3021.0. We will monitor the situation from our side as well.
,
Feb 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1339655a2c52f3e775166de7a21caa92d831c55d commit 1339655a2c52f3e775166de7a21caa92d831c55d Author: chfremer <chfremer@chromium.org> Date: Sat Feb 25 08:02:27 2017 Fix for issue 696098 (potential video capture crash ChromeOS) In a previous CL [1], the contract between VideoCaptureDeviceClient and VideoFrameReceiver has changed such that VideoCaptureDeviceClient is required to call VideoFrameReceiver::OnNewBufferHandle() before using the corresponding buffer to push frames. The call to OnNewBufferHandle() was made in OnIncomingCapturedBufferExt(). However, in the case of accelerated MJPEG decoding, this code path is not actually getting used for the delivery of frames. And the code path that does get used for delivery of accelerated MJPEG frames does not make the required calls to OnNewBufferHandle(). This CL moves the calls to OnNewBufferHandle() to a location that is used in both cases, which is immediately after a buffer is reserved. [1] https://codereview.chromium.org/2686763002/ BUG= 696098 TEST=Currently not covered by any automated test. For manual test, run video capture on a ChromeOS device that uses accelerated MJPEG decoding. Review-Url: https://codereview.chromium.org/2715713006 Cr-Commit-Position: refs/heads/master@{#453077} [modify] https://crrev.com/1339655a2c52f3e775166de7a21caa92d831c55d/media/capture/video/video_capture_device_client.cc
,
Feb 27 2017
Are all platform supporting jpeg hw acceleration affected by this issue?
,
Feb 27 2017
Yes, but as far as I know, hardware accelerated jpeg decode for video capture is only enabled on Chrome OS. Since a patch for this has landed, I am changing the status to fixed.
,
Feb 27 2017
Sorry, I should have said all ChromeOS platforms (BSWL, SKL,...)
,
Feb 27 2017
Oh, now I understand :-). The answer is yes, I believe the issue is independent of which specific ChromeOS platform it is. Should affect all of them in the same way.
,
Feb 27 2017
Thanks for clarifying! Vin to follow up on if our webcam test automation caught this regression?
,
Feb 27 2017
Doesn't look like this bug affected the video_WebRtcPeerConnectionWithCamera or video_WebRtcCamera tests. video_WebRtcPeerConnectionWithCamera was seeing some failures earlier that seem to have been fixed suspiciously close to when chfremer@'s fix went in, but was likely unrelated since we haven't uprev'd our Chrome version yet.
,
Mar 2 2017
Based on https://bugs.chromium.org/p/chromium/issues/detail?id=696130, it looks like the fix was effective. Great to see that we have test coverage that caught this. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 Deleted