New issue
Advanced search Search tips

Issue 696098 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Potential video capture crash in accelerated jpeg decode on ChromeOS

Project Member Reported by chfremer@chromium.org, Feb 25 2017

Issue description

I 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
 

Comment 1 Deleted

The code review is up:
https://codereview.chromium.org/2715713006
Cc: rohi...@chromium.org
Components: OS>Kernel>Video
Labels: videoshortlist
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.
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Cc: vsu...@chromium.org avkodipelli@chromium.org
Are all platform supporting jpeg hw acceleration affected by this issue? 
Status: Fixed (was: Assigned)
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.
Sorry, I should have said all ChromeOS platforms (BSWL, SKL,...)
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.
Thanks for clarifying!

Vin to follow up on if our webcam test automation caught this regression?
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.
Status: Verified (was: Fixed)
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