Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624213006:
media: Fix MojoAudioDecoder reinitialization (Closed)
|
Created:
3 years, 11 months ago by xhwang Modified:
3 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, alokp+watch_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Fix MojoAudioDecoder reinitialization
BUG=679095
TEST=New test added.
Review-Url: https://codereview.chromium.org/2624213006
Cr-Commit-Position: refs/heads/master@{#444888}
Committed: https://chromium.googlesource.com/chromium/src/+/2a02ac7ae09857022954bd2627f74416e683fce3
Patch Set 1 #
Total comments: 5
Patch Set 2 : rebase only #Patch Set 3 : drop extra flags #
Total comments: 2
Patch Set 4 : comments addressed #Patch Set 5 : more tests #Patch Set 6 : add dcheck and more tests #
Messages
Total messages: 29 (14 generated)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xhwang@chromium.org changed reviewers: + sandersd@chromium.org
sandersd: This change basically copied what you did in MojoVideoDecoder for consistency. PTAL!
lgtm
xhwang@chromium.org changed reviewers: + dcheng@chromium.org
dcheng: Please OWNERS review the trivial change in the mojom file. Which basically makes it consistent with video_decoder.mojom. See https://cs.chromium.org/chromium/src/media/mojo/interfaces/video_decoder.mojo...
dcheng: post-weekend kindly ping :)
https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... File media/mojo/clients/mojo_audio_decoder.cc (right): https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... media/mojo/clients/mojo_audio_decoder.cc:46: if (!remote_decoder_bound_) One thing I'd like to understand is why we have these bools (remote_decoder_bound_ and has_connection_error_) instead of just checking the status of the InterfacePtr object. It seems like they could get out of sync, and it's not 100% clear what they refer too either (in the case of has_connection_error_). I would prefer we just check is_bound() on the InterfacePtr directly.
https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... File media/mojo/clients/mojo_audio_decoder.cc (right): https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... media/mojo/clients/mojo_audio_decoder.cc:46: if (!remote_decoder_bound_) On 2017/01/17 19:48:10, dcheng wrote: > One thing I'd like to understand is why we have these bools > (remote_decoder_bound_ and has_connection_error_) instead of just checking the > status of the InterfacePtr object. It seems like they could get out of sync, and > it's not 100% clear what they refer too either (in the case of > has_connection_error_). I would prefer we just check is_bound() on the > InterfacePtr directly. In this case (and also the code it is copied from), it's more about tracking whether the method (BindRemoteDecoder()) was called, and less about the state of the binding. Unbinding the interface should not make us call the method again, for example. That said, they currently have the same meaning, so it might be an improvement to simplify it down to one concept until such time as the meanings diverge.
https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... File media/mojo/clients/mojo_audio_decoder.cc (right): https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... media/mojo/clients/mojo_audio_decoder.cc:46: if (!remote_decoder_bound_) > I would prefer we just check is_bound() on the InterfacePtr directly. After reading through the implementation, I'm not sure if an InterfacePtr can become unbound on its own. If that's possible, are there any uses of the InterfacePtr that would become invalid at the same time?
https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... File media/mojo/clients/mojo_audio_decoder.cc (right): https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... media/mojo/clients/mojo_audio_decoder.cc:46: if (!remote_decoder_bound_) On 2017/01/17 20:12:28, sandersd wrote: > > I would prefer we just check is_bound() on the InterfacePtr directly. > > After reading through the implementation, I'm not sure if an InterfacePtr can > become unbound on its own. If that's possible, are there any uses of the > InterfacePtr that would become invalid at the same time? I confirmed with the mojo team, but a connection error does not unbind. Since both of these bits can be tracked with information already available on InterfacePtr, I think it would be better if we just use what Mojo already has. WDYT?
rebase only
PTAL again! https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... File media/mojo/clients/mojo_audio_decoder.cc (right): https://codereview.chromium.org/2624213006/diff/1/media/mojo/clients/mojo_aud... media/mojo/clients/mojo_audio_decoder.cc:46: if (!remote_decoder_bound_) On 2017/01/18 01:39:18, dcheng wrote: > On 2017/01/17 20:12:28, sandersd wrote: > > > I would prefer we just check is_bound() on the InterfacePtr directly. > > > > After reading through the implementation, I'm not sure if an InterfacePtr can > > become unbound on its own. If that's possible, are there any uses of the > > InterfacePtr that would become invalid at the same time? > > I confirmed with the mojo team, but a connection error does not unbind. > Since both of these bits can be tracked with information already available on > InterfacePtr, I think it would be better if we just use what Mojo already has. > WDYT? Thanks for the suggestions. This makes sense to me. Done.
mojo lgtm https://codereview.chromium.org/2624213006/diff/40001/media/mojo/clients/mojo... File media/mojo/clients/mojo_audio_decoder.cc (right): https://codereview.chromium.org/2624213006/diff/40001/media/mojo/clients/mojo... media/mojo/clients/mojo_audio_decoder.cc:133: DCHECK(!remote_decoder_.is_bound()); FWIW, this DCHECK and the DCHECK on line 136 are in Mojo as well (see BindInternal() in binding_state.cc, which DCHECKS that there's no router_ initially, and then allocates router_ with new. So this isn't strictly necessary in my opinion.
comments addressed
https://codereview.chromium.org/2624213006/diff/40001/media/mojo/clients/mojo... File media/mojo/clients/mojo_audio_decoder.cc (right): https://codereview.chromium.org/2624213006/diff/40001/media/mojo/clients/mojo... media/mojo/clients/mojo_audio_decoder.cc:133: DCHECK(!remote_decoder_.is_bound()); On 2017/01/19 00:43:57, dcheng wrote: > FWIW, this DCHECK and the DCHECK on line 136 are in Mojo as well (see > BindInternal() in binding_state.cc, which DCHECKS that there's no router_ > initially, and then allocates router_ with new. So this isn't strictly necessary > in my opinion. Done.
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2624213006/#ps60001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484867799466410,
"parent_rev": "d43836ab1e9e7d511128f58e0009966b1ee78ffe", "commit_rev":
"2a02ac7ae09857022954bd2627f74416e683fce3"}
Message was sent while issue was closed.
Description was changed from ========== media: Fix MojoAudioDecoder reinitialization BUG=679095 TEST=New test added. ========== to ========== media: Fix MojoAudioDecoder reinitialization BUG=679095 TEST=New test added. Review-Url: https://codereview.chromium.org/2624213006 Cr-Commit-Position: refs/heads/master@{#444888} Committed: https://chromium.googlesource.com/chromium/src/+/2a02ac7ae09857022954bd2627f7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2a02ac7ae09857022954bd2627f7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
