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

Issue 2841553003: media: Discard the previous decoder immediately on fallback (Closed)

Created:
3 years, 8 months ago by watk
Modified:
3 years, 8 months ago
Reviewers:
tguilbert
CC:
sandersd (OOO until July 31), chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

media: Discard the previous decoder immediately on fallback Previously, after falling back to another decoder, we had to keep the old one alive in case it owned frames that were currently being displayed. Now all decoders produce frames that outlive themselves, so we can safely delete the decoder as soon as we no longer need it. This change also includes deletion of a related, but now unused, VideoFrame metadata value called DECODER_OWNS_FRAME. BUG=663988 Review-Url: https://codereview.chromium.org/2841553003 Cr-Commit-Position: refs/heads/master@{#466854} Committed: https://chromium.googlesource.com/chromium/src/+/d16bb3ef495de7c6bdeb57d44c5353273becdcda

Patch Set 1 #

Patch Set 2 : Remove out of date comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -56 lines) Patch
M media/base/video_frame_metadata.h View 1 chunk +0 lines, -6 lines 0 comments Download
M media/blink/webmediaplayer_impl.cc View 1 2 chunks +2 lines, -6 lines 0 comments Download
M media/filters/decoder_stream.h View 2 chunks +2 lines, -10 lines 0 comments Download
M media/filters/decoder_stream.cc View 8 chunks +7 lines, -20 lines 1 comment Download
M media/filters/gpu_video_decoder.cc View 1 chunk +0 lines, -3 lines 0 comments Download
M media/filters/video_frame_stream_unittest.cc View 2 chunks +1 line, -8 lines 0 comments Download
M media/renderers/video_overlay_factory.cc View 1 chunk +0 lines, -3 lines 0 comments Download

Messages

Total messages: 15 (9 generated)
watk
3 years, 8 months ago (2017-04-24 23:11:36 UTC) #5
watk
https://codereview.chromium.org/2841553003/diff/20001/media/filters/decoder_stream.cc File media/filters/decoder_stream.cc (left): https://codereview.chromium.org/2841553003/diff/20001/media/filters/decoder_stream.cc#oldcode568 media/filters/decoder_stream.cc:568: DCHECK(pending_demuxer_read_ || state_ == STATE_ERROR) << state_; Two lines ...
3 years, 8 months ago (2017-04-24 23:13:02 UTC) #6
watk
3 years, 8 months ago (2017-04-24 23:14:23 UTC) #7
tguilbert
On 2017/04/24 23:14:23, watk wrote: LGTM
3 years, 8 months ago (2017-04-24 23:40:24 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2841553003/20001
3 years, 8 months ago (2017-04-25 01:03:05 UTC) #12
commit-bot: I haz the power
3 years, 8 months ago (2017-04-25 01:18:46 UTC) #15
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d16bb3ef495de7c6bdeb57d44c53...

Powered by Google App Engine
This is Rietveld 408576698