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

Issue 2429723006: MojoVideoDecoder: Plumb metadata methods. (Closed)

Created:
4 years, 2 months ago by sandersd (OOO until July 31)
Modified:
4 years, 2 months ago
CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, chromium-reviews, darin (slow to review), feature-media-reviews_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MojoVideoDecoder: Plumb metadata methods. This CL plumbs NeedsBitstreamConversion() and GetMaxDecodeRequests(), which will be available after Initialize() completes. BUG=522298 Committed: https://crrev.com/29d0669f53020750e0203ce46a00ba7dfbd9c5fd Cr-Commit-Position: refs/heads/master@{#426705}

Patch Set 1 #

Total comments: 13

Patch Set 2 : Remove |can_read_without_stalling|. #

Total comments: 2

Patch Set 3 : Actually early return. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -51 lines) Patch
M media/base/video_decoder.h View 1 chunk +1 line, -2 lines 0 comments Download
M media/mojo/clients/mojo_video_decoder.h View 1 3 chunks +13 lines, -4 lines 0 comments Download
M media/mojo/clients/mojo_video_decoder.cc View 1 2 7 chunks +45 lines, -27 lines 0 comments Download
M media/mojo/interfaces/video_decoder.mojom View 1 1 chunk +17 lines, -12 lines 0 comments Download
M media/mojo/services/mojo_video_decoder_service.cc View 1 5 chunks +10 lines, -6 lines 0 comments Download

Messages

Total messages: 28 (8 generated)
sandersd (OOO until July 31)
posciak@, bbudge@: FYI for change to video_decoder.h. Please check that the removal of the in-order ...
4 years, 2 months ago (2016-10-18 20:12:39 UTC) #3
dcheng
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc#newcode103 media/mojo/clients/mojo_video_decoder.cc:103: base::Unretained(this), decode_id)); Can we just bind the callback directly ...
4 years, 2 months ago (2016-10-18 20:39:05 UTC) #4
slan
fly-by comment. mostly l-g-t-m https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc#newcode100 media/mojo/clients/mojo_video_decoder.cc:100: pending_decodes_[decode_id] = decode_cb; What if ...
4 years, 2 months ago (2016-10-18 20:46:25 UTC) #6
slan
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc#newcode100 media/mojo/clients/mojo_video_decoder.cc:100: pending_decodes_[decode_id] = decode_cb; On 2016/10/18 20:46:25, slan wrote: > ...
4 years, 2 months ago (2016-10-18 20:49:42 UTC) #7
sandersd (OOO until July 31)
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc#newcode100 media/mojo/clients/mojo_video_decoder.cc:100: pending_decodes_[decode_id] = decode_cb; On 2016/10/18 20:46:25, slan wrote: > ...
4 years, 2 months ago (2016-10-18 21:09:56 UTC) #8
slan
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc#newcode100 media/mojo/clients/mojo_video_decoder.cc:100: pending_decodes_[decode_id] = decode_cb; On 2016/10/18 21:09:56, sandersd wrote: > ...
4 years, 2 months ago (2016-10-18 22:06:44 UTC) #9
tguilbert
Talked offline with sandersd@. I reviewed this keeping in mind potential DecoderStream side-effects, but saw ...
4 years, 2 months ago (2016-10-19 02:16:02 UTC) #10
dcheng
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc#newcode103 media/mojo/clients/mojo_video_decoder.cc:103: base::Unretained(this), decode_id)); On 2016/10/18 20:39:05, dcheng wrote: > Can ...
4 years, 2 months ago (2016-10-19 02:42:40 UTC) #12
Ken Rockot(use gerrit already)
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc#newcode103 media/mojo/clients/mojo_video_decoder.cc:103: base::Unretained(this), decode_id)); On 2016/10/19 at 02:42:39, dcheng wrote: > ...
4 years, 2 months ago (2016-10-19 05:50:34 UTC) #13
sandersd (OOO until July 31)
> In this case I think maybe it's worth considering whether MojoVideoDecoder > really needs ...
4 years, 2 months ago (2016-10-19 17:51:02 UTC) #14
sandersd (OOO until July 31)
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_video_decoder.cc#newcode118 media/mojo/clients/mojo_video_decoder.cc:118: DCHECK(pending_decodes_.size() > 1 || can_read_without_stalling); On 2016/10/19 02:42:39, dcheng ...
4 years, 2 months ago (2016-10-19 17:51:24 UTC) #15
sandersd (OOO until July 31)
I've removed the |can_read_without_stalling| code, pending a decoder that actually needs it for design guidance. ...
4 years, 2 months ago (2016-10-19 18:28:30 UTC) #16
bbudge
On 2016/10/18 20:12:39, sandersd wrote: > posciak@, bbudge@: FYI for change to video_decoder.h. Please check ...
4 years, 2 months ago (2016-10-19 21:46:18 UTC) #17
sandersd (OOO until July 31)
> I think VideoDecoder is only used by the Pepper VideoDecoderShim, which wraps a > ...
4 years, 2 months ago (2016-10-19 21:54:02 UTC) #18
bbudge
On 2016/10/19 21:54:02, sandersd wrote: > > I think VideoDecoder is only used by the ...
4 years, 2 months ago (2016-10-19 22:06:05 UTC) #19
dcheng
LGTM (Sorry I always forget which process media code lives in... maybe some sort of ...
4 years, 2 months ago (2016-10-19 22:18:33 UTC) #20
sandersd (OOO until July 31)
https://codereview.chromium.org/2429723006/diff/20001/media/mojo/clients/mojo_video_decoder.cc File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/20001/media/mojo/clients/mojo_video_decoder.cc#newcode118 media/mojo/clients/mojo_video_decoder.cc:118: Stop(); On 2016/10/19 22:18:32, dcheng wrote: > This needs ...
4 years, 2 months ago (2016-10-19 22:27:54 UTC) #21
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/2429723006/40001
4 years, 2 months ago (2016-10-20 23:45:00 UTC) #24
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 2 months ago (2016-10-21 02:46:08 UTC) #25
commit-bot: I haz the power
4 years, 2 months ago (2016-10-21 13:26:37 UTC) #27
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/29d0669f53020750e0203ce46a00ba7dfbd9c5fd
Cr-Commit-Position: refs/heads/master@{#426705}

Powered by Google App Engine
This is Rietveld 408576698