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

Issue 1815063002: Detect configuration changes in H.264 video streams in the DXVA decoder (Closed)

Created:
4 years, 9 months ago by ananta
Modified:
4 years, 9 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

Detect configuration changes in H.264 video streams in the DXVA decoder The media foundation h.264 decoder has problems handling changes in the H.264 video streams like resolution changes, bitstream changes etc causing it to get into an infinite MF_E_TRANSFORM_STREAM_CHANGE error sequence leading to a crash. If we reinitialize the decoder then it works fine. The fix for this is to track the H.264 SPS/PPS changes and reinitialize the decoder when we detect a change. We use the media::H264Parser class to parse the stream and retrieve the SPS/PPS structures. BUG=594266 Committed: https://crrev.com/e219f13c3619c1477c815f1db22aa4456d4d5f1c Cr-Commit-Position: refs/heads/master@{#382168}

Patch Set 1 #

Patch Set 2 : Revert some changes #

Patch Set 3 : Check error from the CheckConfigChanged function and bail. #

Patch Set 4 : Moved the H.264 stream parsing code to a class H264ConfigChangeDetector #

Patch Set 5 : Revert unneeded code #

Patch Set 6 : Report config changed only after we see an IDR frame #

Total comments: 2

Patch Set 7 : Flag a pending configuration change if we detect a change in SPS/PPS and return config changed the … #

Unified diffs Side-by-side diffs Delta from patch set Stats (+270 lines, -16 lines) Patch
M content/common/gpu/media/dxva_video_decode_accelerator_win.h View 1 2 3 4 5 6 5 chunks +59 lines, -5 lines 0 comments Download
M content/common/gpu/media/dxva_video_decode_accelerator_win.cc View 1 2 3 4 5 6 11 chunks +211 lines, -11 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 17 (3 generated)
ananta
4 years, 9 months ago (2016-03-18 01:50:44 UTC) #2
sandersd (OOO until July 31)
Overall looks good, please move the new methods into a helper class to ease merging ...
4 years, 9 months ago (2016-03-18 22:47:57 UTC) #3
ananta
On 2016/03/18 22:47:57, sandersd wrote: > Overall looks good, please move the new methods into ...
4 years, 9 months ago (2016-03-18 22:53:18 UTC) #4
sandersd (OOO until July 31)
lgtm
4 years, 9 months ago (2016-03-18 22:57:21 UTC) #5
sandersd (OOO until July 31)
undo lgtm This CL is missing the logic to restrict to keyframes, and is vulnerable ...
4 years, 9 months ago (2016-03-18 22:58:23 UTC) #6
sandersd (OOO until July 31)
On 2016/03/18 22:58:23, sandersd wrote: > undo lgtm > > This CL is missing the ...
4 years, 9 months ago (2016-03-18 22:58:38 UTC) #7
sandersd (OOO until July 31)
Okay, after reviewing again, the cases where this difference matters are unlikely. We should check ...
4 years, 9 months ago (2016-03-18 23:01:53 UTC) #8
ananta
On 2016/03/18 23:01:53, sandersd wrote: > Okay, after reviewing again, the cases where this difference ...
4 years, 9 months ago (2016-03-18 23:22:11 UTC) #9
sandersd (OOO until July 31)
https://codereview.chromium.org/1815063002/diff/100001/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/1815063002/diff/100001/content/common/gpu/media/dxva_video_decode_accelerator_win.cc#newcode445 content/common/gpu/media/dxva_video_decode_accelerator_win.cc:445: if (!last_sps_.empty() && idr_seen) I worry that Flash may ...
4 years, 9 months ago (2016-03-18 23:38:29 UTC) #10
ananta
https://codereview.chromium.org/1815063002/diff/100001/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/1815063002/diff/100001/content/common/gpu/media/dxva_video_decode_accelerator_win.cc#newcode445 content/common/gpu/media/dxva_video_decode_accelerator_win.cc:445: if (!last_sps_.empty() && idr_seen) On 2016/03/18 23:38:29, sandersd wrote: ...
4 years, 9 months ago (2016-03-19 00:00:59 UTC) #11
sandersd (OOO until July 31)
lgtm
4 years, 9 months ago (2016-03-19 00:38:14 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1815063002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1815063002/120001
4 years, 9 months ago (2016-03-19 01:18:02 UTC) #14
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 9 months ago (2016-03-19 01:28:19 UTC) #15
commit-bot: I haz the power
4 years, 9 months ago (2016-03-19 01:29:13 UTC) #17
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/e219f13c3619c1477c815f1db22aa4456d4d5f1c
Cr-Commit-Position: refs/heads/master@{#382168}

Powered by Google App Engine
This is Rietveld 408576698