Detect configuration changes in H.264 video streams in the DXVA decoder

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}

[ananta, 2016-03-18 01:50:43]

ananta@chromium.org changed reviewers:
+ sandersd@chromium.org

[sandersd (OOO until July 31), 2016-03-18 22:47:57]

Overall looks good, please move the new methods into a helper class to ease
merging with the VTVDA implementation later.

[ananta, 2016-03-18 22:53:18]

On 2016/03/18 22:47:57, sandersd wrote:
> Overall looks good, please move the new methods into a helper class to ease
> merging with the VTVDA implementation later.

Done. PTAL

[sandersd (OOO until July 31), 2016-03-18 22:57:21]

lgtm

[sandersd (OOO until July 31), 2016-03-18 22:58:23]

undo lgtm

This CL is missing the logic to restrict to keyframes, and is vulnerable to
changes that are not activated by a slice.

[sandersd (OOO until July 31), 2016-03-18 22:58:38]

On 2016/03/18 22:58:23, sandersd wrote:
> undo lgtm
> 
> This CL is missing the logic to restrict to keyframes, and is vulnerable to
> changes that are not activated by a slice.

no lgtm?

not lgtm?

[sandersd (OOO until July 31), 2016-03-18 23:01:53]

Okay, after reviewing again, the cases where this difference matters are
unlikely. We should check that the pps/sps IDs in the slice headers match the
detector, and verify that were not changing the config at non-keyframes though.

For now, I would say just change the change detector to return false until it
sees an IDR slice.

[ananta, 2016-03-18 23:22:11]

On 2016/03/18 23:01:53, sandersd wrote:
> Okay, after reviewing again, the cases where this difference matters are
> unlikely. We should check that the pps/sps IDs in the slice headers match the
> detector, and verify that were not changing the config at non-keyframes
though.
> 
> For now, I would say just change the change detector to return false until it
> sees an IDR slice.

Done. PTAL

[sandersd (OOO until July 31), 2016-03-18 23:38:29]

https://codereview.chromium.org/1815063002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/1815063002/diff/100001/content/common/gpu/med...
content/common/gpu/media/dxva_video_decode_accelerator_win.cc:445: if
(!last_sps_.empty() && idr_seen)
I worry that Flash may send the SPS/PPS on one buffer, then the IDR slice in the
next buffer. Perhaps better would be |pending_config_changed_|, and then promote
that to |config_changed_| on IDR?

(That's a bit of a rabbit hole, since presumably you would also want a
pending_sps_ and pending_pps_ so that if it 'changes back' you could drop the
bit. I wouldn't worry about that for now.)

[ananta, 2016-03-19 00:00:59]

https://codereview.chromium.org/1815063002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/1815063002/diff/100001/content/common/gpu/med...
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:
> I worry that Flash may send the SPS/PPS on one buffer, then the IDR slice in
the
> next buffer. Perhaps better would be |pending_config_changed_|, and then
promote
> that to |config_changed_| on IDR?
> 
> (That's a bit of a rabbit hole, since presumably you would also want a
> pending_sps_ and pending_pps_ so that if it 'changes back' you could drop the
> bit. I wouldn't worry about that for now.)

Thanks for pointing that out. Updated

[sandersd (OOO until July 31), 2016-03-19 00:38:14]

lgtm

[ananta, 2016-03-19 01:17:55]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2016-03-19 01:18:02]

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

[commit-bot: I haz the power, 2016-03-19 01:28:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-19 01:29:12]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-03-19 01:29:13]

Patchset 7 (id:??) landed as
https://crrev.com/e219f13c3619c1477c815f1db22aa4456d4d5f1c
Cr-Commit-Position: refs/heads/master@{#382168}