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

Issue 1666043002: Attempt to fix a crash in the H.264 decoder on Windows in the GPU process. (Closed)

Created:
4 years, 10 months ago by ananta
Modified:
4 years, 10 months ago
Reviewers:
DaleCurtis
CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, piman+watch_chromium.org, posciak+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 H.264 decoder on Windows in the GPU process. This crash occurs in the DXVAVideoDecodeAccelerator::Decode functions in a number of places. 1. While allocating an IMFSample interface for the packet coming in. 2. While copying the encoded data into the the sample. 3. While posting a task to the decoder thread. It is unclear as to why these crashes occur. We have not been able to reproduce these in house. The callstack in the dump for 1 above, indicates that we are trying to allocate a sample for the default stream size of 0x1000, while the incoming packet is much lesser than that. I don't think that this crash is due to OOM. However it seems better to allocate what is needed. The other change is to set the current length on the media buffer before unlocking it as per msdn guidelines. BUG=455390 Committed: https://crrev.com/fc09e24ec81ada1e5e462ff92d0eec1ecf38fe80 Cr-Commit-Position: refs/heads/master@{#373684}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Use uint32_t for size #

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

Depends on Patchset:

Messages

Total messages: 14 (5 generated)
ananta
4 years, 10 months ago (2016-02-04 02:21:17 UTC) #2
DaleCurtis
https://codereview.chromium.org/1666043002/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/1666043002/diff/1/content/common/gpu/media/dxva_video_decode_accelerator_win.cc#newcode258 content/common/gpu/media/dxva_video_decode_accelerator_win.cc:258: std::min<int>(bitstream_buffer.size(), stream_size), seems this should be uint32_t or DWORD, ...
4 years, 10 months ago (2016-02-04 22:13:18 UTC) #3
ananta
https://codereview.chromium.org/1666043002/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/1666043002/diff/1/content/common/gpu/media/dxva_video_decode_accelerator_win.cc#newcode258 content/common/gpu/media/dxva_video_decode_accelerator_win.cc:258: std::min<int>(bitstream_buffer.size(), stream_size), On 2016/02/04 22:13:18, DaleCurtis wrote: > seems ...
4 years, 10 months ago (2016-02-04 22:32:21 UTC) #4
DaleCurtis
lgtm
4 years, 10 months ago (2016-02-04 22:41:23 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666043002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666043002/20001
4 years, 10 months ago (2016-02-04 23:34:21 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/17691)
4 years, 10 months ago (2016-02-04 23:44:30 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666043002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666043002/20001
4 years, 10 months ago (2016-02-05 00:16:34 UTC) #11
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 10 months ago (2016-02-05 00:40:59 UTC) #12
commit-bot: I haz the power
4 years, 10 months ago (2016-02-05 00:42:32 UTC) #14
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/fc09e24ec81ada1e5e462ff92d0eec1ecf38fe80
Cr-Commit-Position: refs/heads/master@{#373684}

Powered by Google App Engine
This is Rietveld 408576698