Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(482)

Issue 1591603002: Attempt to fix a crash in the DXVA decoder in the DXVAVideoDecodeAccelerator::ProcessPendingSamplesā€¦ (Closed)

Created:
4 years, 11 months ago by ananta
Modified:
4 years, 11 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Attempt to fix a crash in the DXVA decoder in the DXVAVideoDecodeAccelerator::ProcessPendingSamples function. Based on the crash dump, the crash occurs after the call to the CopyOutputSampleDataToPictureBuffer function. This function attempts to copy the decoded texture out to the target surface. Prior to doing that it initializes the video format converter object, which essentially means setting some attributes on the object, setting its input and output media types. If the initialization of the converter fails, then this results in a call to the RETURN_AND_NOTIFY_ON_FAILURE macro which internally invalidates the decoder. The crash occurs on return from here as the iterator is now invalid. Proposed fixes are as below:-. 1. Move the setting of the MF_XVP_PLAYBACK_MODE and MF_LOW_LATENCY attributes on the format converter to the initialization code. This should ensure that we fail early if these attributes cannot be set on the converter. 2. Set the minimum attributes on the media type in the converter. These include the MFMediaType_Video, MF_MT_SUBTYPE and MF_MT_FRAME_SIZE attributes. The rest are not needed. 3. Try to fallback to MFVideoFormat_RGB32 if we fail to find MFVideoFormat_ARGB32 as a supported output type in the converter. If the above steps fail, then we have some CHECK's in the code which should help catch and analyze crashes in this code path. BUG=495216 Committed: https://crrev.com/734cc3d830754f794d576db55f54f77c958315d7 Cr-Commit-Position: refs/heads/master@{#369882}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -83 lines) Patch
M content/common/gpu/media/dxva_video_decode_accelerator_win.h View 1 chunk +11 lines, -0 lines 0 comments Download
M content/common/gpu/media/dxva_video_decode_accelerator_win.cc View 1 5 chunks +79 lines, -83 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 13 (6 generated)
ananta
4 years, 11 months ago (2016-01-15 03:29:23 UTC) #2
DaleCurtis
=>sandersd
4 years, 11 months ago (2016-01-15 18:21:26 UTC) #4
sandersd (OOO until July 31)
lgtm % nits. https://codereview.chromium.org/1591603002/diff/1/content/common/gpu/media/dxva_video_decode_accelerator_win.cc File content/common/gpu/media/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/1591603002/diff/1/content/common/gpu/media/dxva_video_decode_accelerator_win.cc#newcode2223 content/common/gpu/media/dxva_video_decode_accelerator_win.cc:2223: CHECK(false); Nit: Add an error message ...
4 years, 11 months ago (2016-01-15 19:25:02 UTC) #5
ananta
https://codereview.chromium.org/1591603002/diff/1/content/common/gpu/media/dxva_video_decode_accelerator_win.cc File content/common/gpu/media/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/1591603002/diff/1/content/common/gpu/media/dxva_video_decode_accelerator_win.cc#newcode2223 content/common/gpu/media/dxva_video_decode_accelerator_win.cc:2223: CHECK(false); On 2016/01/15 19:25:02, sandersd wrote: > Nit: Add ...
4 years, 11 months ago (2016-01-15 20:23:41 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591603002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591603002/20001
4 years, 11 months ago (2016-01-15 22:52:06 UTC) #10
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 11 months ago (2016-01-15 23:50:50 UTC) #11
commit-bot: I haz the power
4 years, 11 months ago (2016-01-15 23:51:55 UTC) #13
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/734cc3d830754f794d576db55f54f77c958315d7
Cr-Commit-Position: refs/heads/master@{#369882}

Powered by Google App Engine
This is Rietveld 408576698