|
|
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. |
DescriptionMojoVideoDecoder: 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. #
Messages
Total messages: 28 (8 generated)
sandersd@chromium.org changed reviewers: + bbudge@chromium.org, dcheng@chromium.org, posciak@google.com, tguilbert@chromium.org
sandersd@chromium.org changed required reviewers: + dcheng@chromium.org, tguilbert@chromium.org
posciak@, bbudge@: FYI for change to video_decoder.h. Please check that the removal of the in-order requirement does not impact your known use-cases. (Note that GpuVideoDecoder does not enforce this rule, and VideoDecodeAccelerator does not require it, so correct VDAs could already cause this rule to be violated.)
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:103: base::Unretained(this), decode_id)); Can we just bind the callback directly instead of retaining this information in a map? https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:118: DCHECK(pending_decodes_.size() > 1 || can_read_without_stalling); Should these actually be DCHECKs? Presumably, the actually decoder lives in a less privileged process. https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.h (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.h:87: bool max_decode_requests_ = 1; bool?
slan@chromium.org changed reviewers: + slan@chromium.org
fly-by comment. mostly l-g-t-m https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:100: pending_decodes_[decode_id] = decode_cb; What if we bind |decode_cb| directly to OnDecodeDone? This would get rid of the need for pending_decoder_ and decode_counter_: remote_decoder_->Decode( std::move(mojo_buffer), base::Bind(&MojoVideoDecoder::OnDecodeDone, base::Bind(&MojoVideoDecoder::OnDecodeDone, base::Unretained(this), decode_cb)); ... void MojoVideoDecoder::OnDecodeDone( DecodeCB decode_cb, DecodeStatus status, bool can_read_without_stalling) { ...
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:100: pending_decodes_[decode_id] = decode_cb; On 2016/10/18 20:46:25, slan wrote: > What if we bind |decode_cb| directly to OnDecodeDone? This would get rid of the > need for pending_decoder_ and decode_counter_: > > remote_decoder_->Decode( > std::move(mojo_buffer), > base::Bind(&MojoVideoDecoder::OnDecodeDone, > base::Bind(&MojoVideoDecoder::OnDecodeDone, > base::Unretained(this), decode_cb)); Sorry, this is a typo, you know what I mean :) > > ... > > void MojoVideoDecoder::OnDecodeDone( > DecodeCB decode_cb, > DecodeStatus status, > bool can_read_without_stalling) { > ...
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:100: pending_decodes_[decode_id] = decode_cb; On 2016/10/18 20:46:25, slan wrote: > What if we bind |decode_cb| directly to OnDecodeDone? This would get rid of the > need for pending_decoder_ and decode_counter_: The problem this solves is having a list of outstanding decode callbacks to call in Stop(). It it could be just a std::set of callbacks, that would also work, but I didn't want to go there with a copyable type. https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:118: DCHECK(pending_decodes_.size() > 1 || can_read_without_stalling); On 2016/10/18 20:39:05, dcheng wrote: > Should these actually be DCHECKs? Presumably, the actually decoder lives in a > less privileged process. In fact it lives in a more privileged process (the GPU process). But the point is well taken, this API could conceivably be to a lower-privileged process, and that would be a logical goal in the future. I'm interpreting |can_read_without_stalling| as a developer mistake that we want to catch as soon as possible, but the danger of getting it wrong is not a security one. In actual practice, I expect most decoders to always have |can_read_without_stalling| = true. In both cases it would be safe to call Stop() instead, and that matches the existing behavior of GpuVideoDecoder. Would you prefer I did that instead?
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:100: pending_decodes_[decode_id] = decode_cb; On 2016/10/18 21:09:56, sandersd wrote: > On 2016/10/18 20:46:25, slan wrote: > > What if we bind |decode_cb| directly to OnDecodeDone? This would get rid of > the > > need for pending_decoder_ and decode_counter_: > > The problem this solves is having a list of outstanding decode callbacks to call > in Stop(). It it could be just a std::set of callbacks, that would also work, > but I didn't want to go there with a copyable type. Right, I overlooked that bit. Your method SGTM.
Talked offline with sandersd@. I reviewed this keeping in mind potential DecoderStream side-effects, but saw none. LGTM
dcheng@chromium.org changed reviewers: + rockot@chromium.org
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:103: base::Unretained(this), decode_id)); On 2016/10/18 20:39:05, dcheng wrote: > Can we just bind the callback directly instead of retaining this information in > a map? OK, thanks for the explanation. +rockot, do you know of a lot of other instances of this in the codebase? It seems a bit awkward to have to do this, but if there's no other way... https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:118: DCHECK(pending_decodes_.size() > 1 || can_read_without_stalling); On 2016/10/18 21:09:56, sandersd wrote: > On 2016/10/18 20:39:05, dcheng wrote: > > Should these actually be DCHECKs? Presumably, the actually decoder lives in a > > less privileged process. > > In fact it lives in a more privileged process (the GPU process). But the point > is well taken, this API could conceivably be to a lower-privileged process, and > that would be a logical goal in the future. > > I'm interpreting |can_read_without_stalling| as a developer mistake that we want > to catch as soon as possible, but the danger of getting it wrong is not a > security one. In actual practice, I expect most decoders to always have > |can_read_without_stalling| = true. > > In both cases it would be safe to call Stop() instead, and that matches the > existing behavior of GpuVideoDecoder. Would you prefer I did that instead? Hmm, can you link to the GpuVideoDecoder you're referring to?
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.cc:103: base::Unretained(this), decode_id)); On 2016/10/19 at 02:42:39, dcheng wrote: > On 2016/10/18 20:39:05, dcheng wrote: > > Can we just bind the callback directly instead of retaining this information in > > a map? > > OK, thanks for the explanation. +rockot, do you know of a lot of other instances of this in the codebase? It seems a bit awkward to have to do this, but if there's no other way... It's not a pattern that has come up often. This approach seems awkward but reasonable. Another way it's been solved is to have a scoped callback type which runs with some default arguments if destroyed before being explicitly run. That might be seen as a bit too magical though. In this case I think maybe it's worth considering whether MojoVideoDecoder really needs to take responsibility for ensuring all the callbacks are called. Could it just expose some other way of subscribing to a generic "The MojoVideoDecoder is going away now" event, and clients who care can care?
> In this case I think maybe it's worth considering whether MojoVideoDecoder > really needs to take responsibility for ensuring all the callbacks are called. > Could it just expose some other way of subscribing to a generic "The > MojoVideoDecoder is going away now" event, and clients who care can care? I agree that that's an overall better strategy, but this CL is not the place to be significantly refactoring the media::VideoDecoder interface.
https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... 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 wrote: > On 2016/10/18 21:09:56, sandersd wrote: > > On 2016/10/18 20:39:05, dcheng wrote: > > > Should these actually be DCHECKs? Presumably, the actually decoder lives in > a > > > less privileged process. > > > > In fact it lives in a more privileged process (the GPU process). But the point > > is well taken, this API could conceivably be to a lower-privileged process, > and > > that would be a logical goal in the future. > > > > I'm interpreting |can_read_without_stalling| as a developer mistake that we > want > > to catch as soon as possible, but the danger of getting it wrong is not a > > security one. In actual practice, I expect most decoders to always have > > |can_read_without_stalling| = true. > > > > In both cases it would be safe to call Stop() instead, and that matches the > > existing behavior of GpuVideoDecoder. Would you prefer I did that instead? > > Hmm, can you link to the GpuVideoDecoder you're referring to? https://cs.chromium.org/chromium/src/media/filters/gpu_video_decoder.cc?l=717
I've removed the |can_read_without_stalling| code, pending a decoder that actually needs it for design guidance. PTAL. https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... File media/mojo/clients/mojo_video_decoder.h (right): https://codereview.chromium.org/2429723006/diff/1/media/mojo/clients/mojo_vid... media/mojo/clients/mojo_video_decoder.h:87: bool max_decode_requests_ = 1; On 2016/10/18 20:39:05, dcheng wrote: > bool? Done.
On 2016/10/18 20:12:39, sandersd wrote: > posciak@, bbudge@: FYI for change to video_decoder.h. Please check that the > removal of the in-order requirement does not impact your known use-cases. (Note > that GpuVideoDecoder does not enforce this rule, and VideoDecodeAccelerator does > not require it, so correct VDAs could already cause this rule to be violated.) I think VideoDecoder is only used by the Pepper VideoDecoderShim, which wraps a software decoder to look like a hardware decoder. Glancing at the code, it looks like this class assumes that the Decodes happen in order. However, looking at UMA it appears there are very few usages of the Pepper VideoDecoder API (non-dev, this is different from the one used by Flash) so it's probably OK to go ahead.
> I think VideoDecoder is only used by the Pepper VideoDecoderShim, which wraps a > software decoder to look like a hardware decoder. Glancing at the code, it looks > like this class assumes that the Decodes happen in order. However, looking at > UMA it appears there are very few usages of the Pepper VideoDecoder API > (non-dev, this is different from the one used by Flash) so it's probably OK to > go ahead. I believe that VideoDecoderShim also requires that |output_cb| is called before |decode_done_cb|, which means that it only works with FFmpegDecoder. Should we add this restriction to FFmpegVideoDecoder's header, and explain there that it can be removed by improving VideoDecoderShim? Or is the usage so low that we should be considering removing the feature before that will be a concern?
On 2016/10/19 21:54:02, sandersd wrote: > > I think VideoDecoder is only used by the Pepper VideoDecoderShim, which wraps > a > > software decoder to look like a hardware decoder. Glancing at the code, it > looks > > like this class assumes that the Decodes happen in order. However, looking at > > UMA it appears there are very few usages of the Pepper VideoDecoder API > > (non-dev, this is different from the one used by Flash) so it's probably OK to > > go ahead. > > I believe that VideoDecoderShim also requires that |output_cb| is called before > |decode_done_cb|, which means that it only works with FFmpegDecoder. Should we > add this restriction to FFmpegVideoDecoder's header, and explain there that it > can be removed by improving VideoDecoderShim? Or is the usage so low that we > should be considering removing the feature before that will be a concern? It looks like the shim can instantiate a VpxVideoDecoder or an FFmpegVideoDecoder. https://cs.chromium.org/chromium/src/content/renderer/pepper/video_decoder_sh... How bad is it if the callbacks are called in a scrambled order? UMA shows usage in the ~400 per day range.
LGTM (Sorry I always forget which process media code lives in... maybe some sort of quick guide/documentation/readme would be helpful) https://codereview.chromium.org/2429723006/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_video_decoder.cc:118: Stop(); This needs to return as well; otherwise, it->second below is undefined.
https://codereview.chromium.org/2429723006/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_video_decoder.cc (right): https://codereview.chromium.org/2429723006/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_video_decoder.cc:118: Stop(); On 2016/10/19 22:18:32, dcheng wrote: > This needs to return as well; otherwise, it->second below is undefined. Done. (Oops.)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tguilbert@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2429723006/#ps40001 (title: "Actually early return.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MojoVideoDecoder: Plumb metadata methods. This CL plumbs NeedsBitstreamConversion() and GetMaxDecodeRequests(), which will be available after Initialize() completes, and CanReadWithoutStalling(), which is updated each time a Decode() completes. BUG=522298 ========== to ========== MojoVideoDecoder: Plumb metadata methods. This CL plumbs NeedsBitstreamConversion() and GetMaxDecodeRequests(), which will be available after Initialize() completes, and CanReadWithoutStalling(), which is updated each time a Decode() completes. BUG=522298 Committed: https://crrev.com/29d0669f53020750e0203ce46a00ba7dfbd9c5fd Cr-Commit-Position: refs/heads/master@{#426705} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/29d0669f53020750e0203ce46a00ba7dfbd9c5fd Cr-Commit-Position: refs/heads/master@{#426705}
Message was sent while issue was closed.
Description was changed from ========== MojoVideoDecoder: Plumb metadata methods. This CL plumbs NeedsBitstreamConversion() and GetMaxDecodeRequests(), which will be available after Initialize() completes, and CanReadWithoutStalling(), which is updated each time a Decode() completes. BUG=522298 Committed: https://crrev.com/29d0669f53020750e0203ce46a00ba7dfbd9c5fd Cr-Commit-Position: refs/heads/master@{#426705} ========== to ========== 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} ========== |