Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1074383002:
Introduce VideoConfig/AudioConfig class for CMA backend (Closed)
|
Created:
5 years, 8 months ago by erickung1 Modified:
5 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, lcwu+watch_chromium.org, gunsch+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce VideoConfig/AudioConfig class for CMA backend
1. Replace media::VideoDecoderConfig/media::AudioDecoderConfig by
chromecast::media::VideoConfig/chromecast::media::AudioConfig
2. Create DecoderConfigAdapter class to do the class coversion
3. SetConfig(..) CMA interface to use new class to pass data to
CMA backend
BUG=476126
Committed: https://crrev.com/548865f764227f0755536921d2533f7dcc3de6ae
Cr-Commit-Position: refs/heads/master@{#328611}
Patch Set 1 #
Total comments: 12
Patch Set 2 : single DecoderConfig structure for both Audio and Video #
Total comments: 16
Patch Set 3 : Split the strcutre into {Audio|Video}Config #
Total comments: 19
Patch Set 4 : Rebase and change code based on comment #Patch Set 5 : Change NULL to nullptr #
Total comments: 2
Patch Set 6 : Remove decoder_config.cc #
Total comments: 9
Patch Set 7 : #Patch Set 8 : #
Total comments: 6
Patch Set 9 : Util function name change #
Total comments: 3
Patch Set 10 : Add TODO comments and fix unit test #
Total comments: 1
Messages
Total messages: 41 (5 generated)
erickung@chromium.org changed reviewers: + gunsch@chromium.org, lcwu@chromium.org
erickung@chromium.org changed reviewers: + servolk@chromium.org
On 2015/04/10 23:08:13, erickung1 wrote: I understand that we want to minimize dependencies for CMA backends, but one concern that I have with this change is that it essentially duplicates audio/video config implementations from Chromium. And this will be a maintenance burden. Is there any way to avoid manual maintenance? Maybe copy the Chromium files over to the CMA directory via a script or something?
Disagreed with Sergey: we shouldn't necessarily need to keep this in sync with the Chromium base class. Only if Chromium class adds a new features *and* the CMA backend needs to take advantage of that feature would we need to add it in both places.
On 2015/04/13 22:31:30, gunsch wrote: > Disagreed with Sergey: we shouldn't necessarily need to keep this in sync with > the Chromium base class. Only if Chromium class adds a new features *and* the > CMA backend needs to take advantage of that feature would we need to add it in > both places. Sergey, does the duplicate effort putting two structures in sync means those enum type information? like Video Codec/Video Profile/Audio Codec/Channel Layout? Or it's more like the structure variable in general? For the first one, in my CL, I minimize such change already by limiting the enum to only three(there were few additions but I don't have that in the new structure). Like what Jessie mentioned, maybe I should try to reduce more enum type (for example, channel layout) and create an util function to convert the enum type, that we can decouple from the code in the long run. But yes, the duplication of enum type will still be an issue in this case. If your concern is more about data structure waste, I didn't consider it in my CL. My CL is more to cut the dependency, and don't pass no-used information to CMA unless it's necessary. maybe gunsch@ and lcwu@ can weight it some feedback.
On 2015/04/13 22:46:32, erickung1 wrote: > On 2015/04/13 22:31:30, gunsch wrote: > > Disagreed with Sergey: we shouldn't necessarily need to keep this in sync with > > the Chromium base class. Only if Chromium class adds a new features *and* the > > CMA backend needs to take advantage of that feature would we need to add it in > > both places. > > Sergey, does the duplicate effort putting two structures in sync means those > enum type information? like Video Codec/Video Profile/Audio Codec/Channel > Layout? Or it's more like the structure variable in general? > > For the first one, in my CL, I minimize such change already by limiting the enum > to only three(there were few additions but I don't have that in the new > structure). > Like what Jessie mentioned, maybe I should try to reduce more enum type (for > example, channel layout) and create an util function to convert the enum type, > that we can decouple from the code in the long run. But yes, the duplication of > enum type will still be an issue in this case. > > If your concern is more about data structure waste, I didn't consider it in my > CL. My CL is more to cut the dependency, and don't pass no-used information to > CMA unless it's necessary. maybe gunsch@ and lcwu@ can weight it some feedback. I think code duplication is bad in any form. With the current CL it looks like we are duplicating the whole implementation of audio/video configs (even including enum values that CMA doesn't support). But even if we kept only what CMA supports here, we'd still have to maintain mappings between the CMA specific structures and Chromium data structures. Also I wonder what are you planning to do about things like base::Callback in CMA interfaces - use std::function?
Callback can/should be replaced with an interface method, e.g.
class VideoClient {
virtual void OnNaturalSizeChanged(const Size&);
}
class VideoPipelineDevice {
virtual void SetVideoClient(VideoClient*) = 0;
}
We do the same thing for the HDMI HAL, and it provides an interface with no
Chromium dependencies.
I left a few comments on the AudioConfig header, but they apply to the entire CL. https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... File chromecast/media/cma/public/audio_config.h (right): https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. update https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:5: #ifndef CHROMECAST_MEDIA_CMA_PUBLIC_AUDIO_CONFIG_H_ move to chromecast/public/media/audio_config.h https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:23: kUnknownAudioCodec = 0, I'd rather stick with only the things that we have, and a simple method to convert from a ::media::AudioDecoderConfig to a chromecast::AudioConfig. https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:125: void Initialize(Codec codec, I don't like all the copies of methods from media/. Why not just have AudioConfig be a pure-data struct (no methods), and we have a method that populates it from ::media::AudioConfig.
https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/base/d... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/base/d... chromecast/media/cma/base/decoder_config_adapter.cc:23: static_cast<AudioConfig::Codec>(config.codec()), It's actually dangerous doing this. I am not sure if we want to always keep an exact same enum with ::media's. I would prefer creating a separate util function to convert (using a switch statement). https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... File chromecast/media/cma/public/audio_config.h (right): https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:19: class AudioConfig { A high-level comment: I would prefer the cast audio/video config to be a simple struct. We don't have to create utility methods for the OEMs. And in our own code, we should be operating on the ::media::{Audio|Video}DecoderConfig. This struct should be used when we push the data to OEM's backend implementation. This also means that we might need to refactor the existing OEM's backend implementation and move the common part to the {audio|video}_pipeline_device.cc, for example.
https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/base/d... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/base/d... chromecast/media/cma/base/decoder_config_adapter.cc:23: static_cast<AudioConfig::Codec>(config.codec()), On 2015/04/13 23:55:42, lcwu1 wrote: > It's actually dangerous doing this. I am not sure if we want to always keep an > exact same enum with ::media's. I would prefer creating a separate util function > to convert (using a switch statement). Done. Create util function to do the conversion using switch https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... File chromecast/media/cma/public/audio_config.h (right): https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/04/13 23:46:20, gunsch wrote: > update Done. merge both VideoConfig/AudioConfig into single DecoderConfig https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:5: #ifndef CHROMECAST_MEDIA_CMA_PUBLIC_AUDIO_CONFIG_H_ On 2015/04/13 23:46:20, gunsch wrote: > move to chromecast/public/media/audio_config.h Done. https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:19: class AudioConfig { On 2015/04/13 23:55:42, lcwu1 wrote: > A high-level comment: I would prefer the cast audio/video config to be a simple > struct. We don't have to create utility methods for the OEMs. And in our own > code, we should be operating on the ::media::{Audio|Video}DecoderConfig. This > struct should be used when we push the data to OEM's backend implementation. > This also means that we might need to refactor the existing OEM's backend > implementation and move the common part to the {audio|video}_pipeline_device.cc, > for example. Done the first part, leave CMA interface refactoring(SetConfig to a higher interface level) later on https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:23: kUnknownAudioCodec = 0, On 2015/04/13 23:46:20, gunsch wrote: > I'd rather stick with only the things that we have, and a simple method to > convert from a ::media::AudioDecoderConfig to a chromecast::AudioConfig. Done. https://codereview.chromium.org/1074383002/diff/1/chromecast/media/cma/public... chromecast/media/cma/public/audio_config.h:125: void Initialize(Codec codec, On 2015/04/13 23:46:20, gunsch wrote: > I don't like all the copies of methods from media/. Why not just have > AudioConfig be a pure-data struct (no methods), and we have a method that > populates it from ::media::AudioConfig. Done.
https://codereview.chromium.org/1074383002/diff/20001/chromecast/DEPS File chromecast/DEPS (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/DEPS#newcode8 chromecast/DEPS:8: "+chromecast/public/media", just add "+chromecast/public", any of our code should be able to use the public APIs/configs https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/base/decoder_config_adapter.cc:37: NOTREACHED(); NOTREACHED is generally considered fatal. Is there any chance we'd hit this ahead of the canPlayType checks? https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/base/decoder_config_adapter.h (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/base/decoder_config_adapter.h:30: static Codec ToCodec(const ::media::AudioCodec audio_codec); since these are all static methods, why not just have them in an anonymous namespace in decoder_config_adapter.cc? https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/audio_pipeline_impl.cc (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/audio_pipeline_impl.cc:133: bool success = two lines is okay, no need to wrap to three https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/media.gyp File chromecast/media/media.gyp (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/media.... chromecast/media/media.gyp:67: '../public/media/decoder_config.cc', This should go in the 'cast_public_api' target. https://codereview.chromium.org/1074383002/diff/20001/chromecast/public/media... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/public/media... chromecast/public/media/decoder_config.h:64: struct DecoderConfig { Out of curiosity, what was the logic for merging the two fields into one struct rather than keeping them separate? ISTM like that loses a small dimension of type safety.
https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/base/decoder_config_adapter.cc:100: return decoder_config; I wonder if this would be an appropriate place to use one of the boolean-based conversion APIs, e.g. bool DecoderConfigAdapter::ToDecoderConfig(const ::media::AudioDecoderConfig& input, DecoderConfig* output); so that the failed conversion is more explicit.
This CL is to split the decoder config into two AudioConfig and VideoConfig https://codereview.chromium.org/1074383002/diff/20001/chromecast/DEPS File chromecast/DEPS (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/DEPS#newcode8 chromecast/DEPS:8: "+chromecast/public/media", On 2015/04/17 01:38:51, gunsch wrote: > just add "+chromecast/public", any of our code should be able to use the public > APIs/configs Done. https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/base/decoder_config_adapter.cc:37: NOTREACHED(); On 2015/04/17 01:38:51, gunsch wrote: > NOTREACHED is generally considered fatal. Is there any chance we'd hit this > ahead of the canPlayType checks? Done. Idea is to notify that the codec is unsupported in new data structure. https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/base/decoder_config_adapter.cc:100: return decoder_config; On 2015/04/17 02:21:26, gunsch wrote: > I wonder if this would be an appropriate place to use one of the boolean-based > conversion APIs, e.g. > > bool DecoderConfigAdapter::ToDecoderConfig(const ::media::AudioDecoderConfig& > input, DecoderConfig* output); > > so that the failed conversion is more explicit. The idea of this util function is to 1. convert from one type to another that doesn't have dependency on chrome/media 2. codec related type will convert to unknown(and become invalid) if cast doesn't support. It's OK to convert invalid to another invalid type. https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/base/decoder_config_adapter.h (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/base/decoder_config_adapter.h:30: static Codec ToCodec(const ::media::AudioCodec audio_codec); On 2015/04/17 01:38:51, gunsch wrote: > since these are all static methods, why not just have them in an anonymous > namespace in decoder_config_adapter.cc? Done. https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/audio_pipeline_impl.cc (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/audio_pipeline_impl.cc:133: bool success = On 2015/04/17 01:38:51, gunsch wrote: > two lines is okay, no need to wrap to three Done. https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/media.gyp File chromecast/media/media.gyp (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/media/media.... chromecast/media/media.gyp:67: '../public/media/decoder_config.cc', On 2015/04/17 01:38:51, gunsch wrote: > This should go in the 'cast_public_api' target. Done. https://codereview.chromium.org/1074383002/diff/20001/chromecast/public/media... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/public/media... chromecast/public/media/decoder_config.h:64: struct DecoderConfig { On 2015/04/17 01:38:51, gunsch wrote: > Out of curiosity, what was the logic for merging the two fields into one struct > rather than keeping them separate? ISTM like that loses a small dimension of > type safety. Done. Slipt to AudioConfig and VideoConfig
You should rebase---I think presubmit will enforce some new things that your CL currently violates. https://codereview.chromium.org/1074383002/diff/20001/chromecast/DEPS File chromecast/DEPS (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/DEPS#newcode8 chromecast/DEPS:8: "+chromecast/public/media", On 2015/04/29 08:52:20, erickung1 wrote: > On 2015/04/17 01:38:51, gunsch wrote: > > just add "+chromecast/public", any of our code should be able to use the > public > > APIs/configs > > Done. Not done https://codereview.chromium.org/1074383002/diff/40001/chromecast/media/cma/ba... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/base/decoder_config_adapter.cc:69: return kH264ScalabBaseline; rename: kH264ScalableBaseline https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... File chromecast/public/media/decoder_config.cc (right): https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:5: #include "chromecast/public/media/decoder_config.h" Should this be "decoder_config.h" or "media/decoder_config.h"? The chromecast/public/ directory should be trying to make no assumptions that involve the chromium root path. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:7: namespace { nit: put anonymous namespace within chromecast::media https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:10: static int kMaxBytesPerSample = 4; const (here and below) https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:15: } // namespace https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:57: return (codec >= kVideoCodecMin && codec <= kVideoCodecMax); nit: unnecessary parens https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.h:5: #ifndef CHROMECAST_MEDIA_CMA_PUBLIC_DECODER_CONFIG_H_ not correct https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.h:8: #include <string> not used https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.h:11: #include "base/basictypes.h" a) basictypes.h is deprecated, use uint8_t instead of uint8 b) no chromium includes in chromecast/public https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.h:81: bool IsValidConfig() const; curious to hear lcwu's thoughts comments here: so far we've had no implementation files linked into our public API, only headers. However, this does seem like a useful utility function and linking it into a shlib would be harmless. What sorts of conventions should we be establishing here? One convention I'd like to offer for discussion: how about we only have static utility methods in our public APIs and no static state? The goal would be to prevent statefulness as best as possible for what we provide to shlibs.
https://codereview.chromium.org/1074383002/diff/20001/chromecast/DEPS File chromecast/DEPS (right): https://codereview.chromium.org/1074383002/diff/20001/chromecast/DEPS#newcode8 chromecast/DEPS:8: "+chromecast/public/media", On 2015/04/29 17:42:52, gunsch wrote: > On 2015/04/29 08:52:20, erickung1 wrote: > > On 2015/04/17 01:38:51, gunsch wrote: > > > just add "+chromecast/public", any of our code should be able to use the > > public > > > APIs/configs > > > > Done. > > Not done This is removed after the rebase https://codereview.chromium.org/1074383002/diff/40001/chromecast/media/cma/ba... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/base/decoder_config_adapter.cc:69: return kH264ScalabBaseline; On 2015/04/29 17:42:52, gunsch wrote: > rename: kH264ScalableBaseline Done. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... File chromecast/public/media/decoder_config.cc (right): https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:5: #include "chromecast/public/media/decoder_config.h" On 2015/04/29 17:42:52, gunsch wrote: > Should this be "decoder_config.h" or "media/decoder_config.h"? The > chromecast/public/ directory should be trying to make no assumptions that > involve the chromium root path. Done. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:7: namespace { On 2015/04/29 17:42:52, gunsch wrote: > nit: put anonymous namespace within chromecast::media Done. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:10: static int kMaxBytesPerSample = 4; On 2015/04/29 17:42:52, gunsch wrote: > const (here and below) Done. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:15: } On 2015/04/29 17:42:52, gunsch wrote: > // namespace Done. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.cc:57: return (codec >= kVideoCodecMin && codec <= kVideoCodecMax); On 2015/04/29 17:42:52, gunsch wrote: > nit: unnecessary parens Done. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.h:5: #ifndef CHROMECAST_MEDIA_CMA_PUBLIC_DECODER_CONFIG_H_ On 2015/04/29 17:42:52, gunsch wrote: > not correct Done. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.h:8: #include <string> On 2015/04/29 17:42:52, gunsch wrote: > not used Done. https://codereview.chromium.org/1074383002/diff/40001/chromecast/public/media... chromecast/public/media/decoder_config.h:11: #include "base/basictypes.h" On 2015/04/29 17:42:52, gunsch wrote: > a) basictypes.h is deprecated, use uint8_t instead of uint8 > b) no chromium includes in chromecast/public > Done.
couple more nits. still would like to have the conversation about whether we need the .cc file in the public API at all, ideally leaning towards finding a way to eliminate it if possible https://codereview.chromium.org/1074383002/diff/80001/chromecast/public/media... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/80001/chromecast/public/media... chromecast/public/media/decoder_config.h:51: kH264Main = 1, why are these values numbered, while the ones above not? https://codereview.chromium.org/1074383002/diff/80001/chromecast/public/media... chromecast/public/media/decoder_config.h:74: nit: extra blank line
https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:84: is_encrypted(false) {} Hmm, do we need these ctors? Would prefer not to define any methods if possible (which I think resolves the Clang style plugin issues you were describing last night). Can we use default initializer? https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:129: inline bool IsValidConfig(const AudioConfig& config) { Per Le-Chun's earlier question, do we actually need this or can we simply verify the config is valid before passing it to the OEM?
https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:84: is_encrypted(false) {} On 2015/05/05 16:54:22, gunsch wrote: > Hmm, do we need these ctors? Would prefer not to define any methods if possible > (which I think resolves the Clang style plugin issues you were describing last > night). Can we use default initializer? please see my next comment. I think we should, however, the current OEM implementation relies on IsValidConfig and tends to reset the structure by creating a new object(which happens in Chromium code everywhere). With ctors and IsValidConfig(), we don't have to change a lot in OEM implementation https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:129: inline bool IsValidConfig(const AudioConfig& config) { On 2015/05/05 16:54:22, gunsch wrote: > Per Le-Chun's earlier question, do we actually need this or can we simply verify > the config is valid before passing it to the OEM? Existing OEM implementaion tends do following 1. keep the latest config in their code for future reference 2. reset the config due to state change by creating a new one(making invalid) 3. use IsValidConfig before further actions because of #1, #2 I think it's better to have this function for now to make less change in OEM implementation part, unless we want to make a guideline to OEM implementation that it is OK to keep a copy but it should honor the code to do proper action when receiving the config.
https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:129: inline bool IsValidConfig(const AudioConfig& config) { On 2015/05/05 17:23:44, erickung1 wrote: > On 2015/05/05 16:54:22, gunsch wrote: > > Per Le-Chun's earlier question, do we actually need this or can we simply > verify > > the config is valid before passing it to the OEM? > > Existing OEM implementaion tends do following > 1. keep the latest config in their code for future reference > 2. reset the config due to state change by creating a new one(making invalid) > 3. use IsValidConfig before further actions because of #1, #2 > > I think it's better to have this function for now to make less change in OEM > implementation part, unless we want to make a guideline to OEM implementation > that it is OK to keep a copy but it should honor the code to do proper action > when receiving the config. I'm hearing arguments that it makes migration easier (SGTM), but not for it being a good long-term solution. I'm okay with having this here under that logic if we also include a TODO to remove it and a note to not add new consumers.
https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:129: inline bool IsValidConfig(const AudioConfig& config) { On 2015/05/05 17:30:35, gunsch wrote: > On 2015/05/05 17:23:44, erickung1 wrote: > > On 2015/05/05 16:54:22, gunsch wrote: > > > Per Le-Chun's earlier question, do we actually need this or can we simply > > verify > > > the config is valid before passing it to the OEM? > > > > Existing OEM implementaion tends do following > > 1. keep the latest config in their code for future reference > > 2. reset the config due to state change by creating a new one(making invalid) > > 3. use IsValidConfig before further actions because of #1, #2 > > > > I think it's better to have this function for now to make less change in OEM > > implementation part, unless we want to make a guideline to OEM implementation > > that it is OK to keep a copy but it should honor the code to do proper action > > when receiving the config. > > I'm hearing arguments that it makes migration easier (SGTM), but not for it > being a good long-term solution. I'm okay with having this here under that logic > if we also include a TODO to remove it and a note to not add new consumers. Done.
https://codereview.chromium.org/1074383002/diff/140001/chromecast/media/cma/b... File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc (left): https://codereview.chromium.org/1074383002/diff/140001/chromecast/media/cma/b... chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:189: gfx::Size natural_size(320, 240); I assume the size info (as well as the video frame format) is not needed by the backend? (Note that it might not be needed by one vendor, but could be needed by another. Just want to confirm with you.) https://codereview.chromium.org/1074383002/diff/140001/chromecast/media/cma/b... File chromecast/media/cma/base/decoder_config_adapter.h (right): https://codereview.chromium.org/1074383002/diff/140001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.h:18: static AudioConfig ToAudioConfig( Maybe rename it to 'ToCastAudioConfig' to make it more self-explanatory? (Ditto for ToVideoConfig.) https://codereview.chromium.org/1074383002/diff/140001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/140001/chromecast/public/medi... chromecast/public/media/decoder_config.h:121: // responsibility. This comment should also be applied to the data member above (additional_config), right? Also you might want to make it clear that 'Consumers of this struct should make a copy of the content pointed to by this pointer if they expect to use the data beyond the function ends'.
https://codereview.chromium.org/1074383002/diff/140001/chromecast/media/cma/b... File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc (left): https://codereview.chromium.org/1074383002/diff/140001/chromecast/media/cma/b... chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:189: gfx::Size natural_size(320, 240); On 2015/05/05 23:46:22, lcwu1 wrote: > I assume the size info (as well as the video frame format) is not needed by the > backend? (Note that it might not be needed by one vendor, but could be needed by > another. Just want to confirm with you.) no existing vendors are needed. Actually they can get such information from the bit stream https://codereview.chromium.org/1074383002/diff/140001/chromecast/media/cma/b... File chromecast/media/cma/base/decoder_config_adapter.h (right): https://codereview.chromium.org/1074383002/diff/140001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.h:18: static AudioConfig ToAudioConfig( On 2015/05/05 23:46:22, lcwu1 wrote: > Maybe rename it to 'ToCastAudioConfig' to make it more self-explanatory? (Ditto > for ToVideoConfig.) Done. https://codereview.chromium.org/1074383002/diff/140001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/140001/chromecast/public/medi... chromecast/public/media/decoder_config.h:121: // responsibility. On 2015/05/05 23:46:22, lcwu1 wrote: > This comment should also be applied to the data member above > (additional_config), right? > > Also you might want to make it clear that 'Consumers of this struct should make > a copy of the content pointed to by this pointer if they expect to use the data > beyond the function ends'. Done.
lgtm
gunsch@google.com changed reviewers: + gunsch@google.com
lgtm % couple small things Btw, please submit this upstream at the same time after internal CLs are also approved, so period of TOT brokenness can be minimized. https://codereview.chromium.org/1074383002/diff/160001/chromecast/media/cma/b... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/160001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:30: LOG(INFO) << "Unsupported audio codec " << audio_codec; should these be LOG(ERROR)s? I looked back and they were NOTREACHED before. It seems to me it's a bug if we're trying to play content we don't support---not even an application bug, but an actual mismatch between the types we've declared support for and trying to push content through the media pipeline. WDYT? https://codereview.chromium.org/1074383002/diff/160001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/160001/chromecast/public/medi... chromecast/public/media/decoder_config.h:133: // is done. These SHOULD NOT be used in New CMA backend implementation. Per your response to my earlier comments, please also add a TODO to remove the struct ctors.
https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:98: // Size of extra data in bytes. Whoa, I just noticed this change... why did we change from std::vector<uint8_t> to uint8_t* + size? That seems like a source of easy mistakes when OEMs are implementing.
https://codereview.chromium.org/1074383002/diff/180001/chromecast/media/cma/b... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/180001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:30: LOG(ERROR) << "Unsupported audio codec " << audio_codec; do you want to add TODO about AC3/EAC3?
https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:98: // Size of extra data in bytes. On 2015/05/06 02:52:35, gunsch wrote: > Whoa, I just noticed this change... why did we change from std::vector<uint8_t> > to uint8_t* + size? That seems like a source of easy mistakes when OEMs are > implementing. using std:vector will require constructor/destructor implementation in .cc file(Chromium C++ style guild). Although we can consider disabling this flag for cast_public_api gyp, allocating memory(memory copy) is not the purposes for the passive object The reason AudioDecoderConfig/VideoDecoderConfig do that is only because it the data won't be huge, using std::vector (memory copy) should be fine. But that's different consideration, from my point of view https://codereview.chromium.org/1074383002/diff/160001/chromecast/media/cma/b... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/1074383002/diff/160001/chromecast/media/cma/b... chromecast/media/cma/base/decoder_config_adapter.cc:30: LOG(INFO) << "Unsupported audio codec " << audio_codec; On 2015/05/06 01:32:42, gunsch-google wrote: > should these be LOG(ERROR)s? I looked back and they were NOTREACHED before. It > seems to me it's a bug if we're trying to play content we don't support---not > even an application bug, but an actual mismatch between the types we've declared > support for and trying to push content through the media pipeline. WDYT? Sure, I'll make this and line #46 to Error, but leave VideoCodecProfile to INFO still since not all video codec has profile.
https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... chromecast/public/media/decoder_config.h:98: // Size of extra data in bytes. On 2015/05/06 03:01:33, erickung1 wrote: > On 2015/05/06 02:52:35, gunsch wrote: > > Whoa, I just noticed this change... why did we change from > std::vector<uint8_t> > > to uint8_t* + size? That seems like a source of easy mistakes when OEMs are > > implementing. > > using std:vector will require constructor/destructor implementation in .cc > file(Chromium C++ style guild). Although we can consider disabling this flag for > cast_public_api gyp, allocating memory(memory copy) is not the purposes for the > passive object > > The reason AudioDecoderConfig/VideoDecoderConfig do that is only because it the > data won't be huge, using std::vector (memory copy) should be fine. But that's > different consideration, from my point of view I'm probably going to insist. The OEM implementation is most likely going to have to memory-copy the std::vector *anyways* (as in your implementation). Assuming that's the case: Arguments against uint8*: our API now passes in a pointer that quickly becomes invalid. If OEM implementation holds onto copies of the AudioConfig/VideoConfig objects (reasonable), they're left with a dangling pointer. Arguments against std::vector: Chromium style guide says we need ctor/dtor implementation in .cc file. To me, that's an easy choice: we should be preferring a better API.
On 2015/05/06 03:08:44, gunsch wrote: > https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... > File chromecast/public/media/decoder_config.h (right): > > https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... > chromecast/public/media/decoder_config.h:98: // Size of extra data in bytes. > On 2015/05/06 03:01:33, erickung1 wrote: > > On 2015/05/06 02:52:35, gunsch wrote: > > > Whoa, I just noticed this change... why did we change from > > std::vector<uint8_t> > > > to uint8_t* + size? That seems like a source of easy mistakes when OEMs are > > > implementing. > > > > using std:vector will require constructor/destructor implementation in .cc > > file(Chromium C++ style guild). Although we can consider disabling this flag > for > > cast_public_api gyp, allocating memory(memory copy) is not the purposes for > the > > passive object > > > > The reason AudioDecoderConfig/VideoDecoderConfig do that is only because it > the > > data won't be huge, using std::vector (memory copy) should be fine. But that's > > different consideration, from my point of view > > I'm probably going to insist. The OEM implementation is most likely going to > have to memory-copy the std::vector *anyways* (as in your implementation). > Assuming that's the case: > > Arguments against uint8*: our API now passes in a pointer that quickly becomes > invalid. If OEM implementation holds onto copies of the AudioConfig/VideoConfig > objects (reasonable), they're left with a dangling pointer. > > Arguments against std::vector: Chromium style guide says we need ctor/dtor > implementation in .cc file. > > To me, that's an easy choice: we should be preferring a better API. I asked Eric to change the use of std:vector to unit8*. Besides the fact that having a C++ data structure would force us to create a separate .cc files which complicates the OEM integration, SoC vendors are traditionally more used to C-style APIs than C++ style APIs. Passing in a pointer to a chunk of data and its size is a very common (C-style_ API design for platform SDKs or OEM layer software. (See the PlayReady SDK APIs and you will find this common pattern.) What we will need to do is to make sure we document clearly that the implementation will need to copy the data explicitly if the data is going to be used beyond the end of the function.
On 2015/05/06 03:57:33, lcwu1 wrote: > On 2015/05/06 03:08:44, gunsch wrote: > > > https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... > > File chromecast/public/media/decoder_config.h (right): > > > > > https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... > > chromecast/public/media/decoder_config.h:98: // Size of extra data in bytes. > > On 2015/05/06 03:01:33, erickung1 wrote: > > > On 2015/05/06 02:52:35, gunsch wrote: > > > > Whoa, I just noticed this change... why did we change from > > > std::vector<uint8_t> > > > > to uint8_t* + size? That seems like a source of easy mistakes when OEMs > are > > > > implementing. > > > > > > using std:vector will require constructor/destructor implementation in .cc > > > file(Chromium C++ style guild). Although we can consider disabling this flag > > for > > > cast_public_api gyp, allocating memory(memory copy) is not the purposes for > > the > > > passive object > > > > > > The reason AudioDecoderConfig/VideoDecoderConfig do that is only because it > > the > > > data won't be huge, using std::vector (memory copy) should be fine. But > that's > > > different consideration, from my point of view > > > > I'm probably going to insist. The OEM implementation is most likely going to > > have to memory-copy the std::vector *anyways* (as in your implementation). > > Assuming that's the case: > > > > Arguments against uint8*: our API now passes in a pointer that quickly becomes > > invalid. If OEM implementation holds onto copies of the > AudioConfig/VideoConfig > > objects (reasonable), they're left with a dangling pointer. > > > > Arguments against std::vector: Chromium style guide says we need ctor/dtor > > implementation in .cc file. > > > > To me, that's an easy choice: we should be preferring a better API. > > I asked Eric to change the use of std:vector to unit8*. Besides the fact that > having a C++ data structure would force us to create a separate .cc files which > complicates the OEM integration, SoC vendors are traditionally more used to > C-style APIs than C++ style APIs. Passing in a pointer to a chunk of data and > its size is a very common (C-style_ API design for platform SDKs or OEM layer > software. (See the PlayReady SDK APIs and you will find this common pattern.) > > What we will need to do is to make sure we document clearly that the > implementation will need to copy the data explicitly if the data is going to be > used beyond the end of the function. Also if you look at our existing cma backend code, after receiving the buffers, we took the address of the buffer data and its size and call the OEM SDK/device drive APIs (which are C APIs) with them anyway.
Couple thought which may not be covered in this CL
1. Current OEM implementation shouldn't hold the object which is passed by the
API after the function is out of scope. It should do all the necessary action
inside the API call. If OEM wants to hold on certain information, it should find
its own data type/structure to keep such information.
2. |extra_data| is a buffer data which should not be counted as part of setup
parameter. We have it right now because of vorbis audio decoder is
required.(althought I'm not sure if there is a cast app using it or not)
3. Ideally, OEM implementation should be cleaned up based on the guideline
mentioned above. I would consider doing that as a separated task
4. It's OK to say we should allow cast_shell and CMA backend using
{Audio|Video}Config data structure without worrying about pointer issue. In that
case, we should consider splitting |extra_data| out from the structure and
propose a new API for it. However, it's low priority thing due to point #2
I think the 4 points above are the RIGHT thing that we should be doing. We
didn't consider doing it merely because the old data structure hides all the
issue and there was never a clear boundary since the initial development on both
sides were done all by us.
On 2015/05/06 04:17:46, lcwu1 wrote: > On 2015/05/06 03:57:33, lcwu1 wrote: > > On 2015/05/06 03:08:44, gunsch wrote: > > > > > > https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... > > > File chromecast/public/media/decoder_config.h (right): > > > > > > > > > https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... > > > chromecast/public/media/decoder_config.h:98: // Size of extra data in bytes. > > > On 2015/05/06 03:01:33, erickung1 wrote: > > > > On 2015/05/06 02:52:35, gunsch wrote: > > > > > Whoa, I just noticed this change... why did we change from > > > > std::vector<uint8_t> > > > > > to uint8_t* + size? That seems like a source of easy mistakes when OEMs > > are > > > > > implementing. > > > > > > > > using std:vector will require constructor/destructor implementation in .cc > > > > file(Chromium C++ style guild). Although we can consider disabling this > flag > > > for > > > > cast_public_api gyp, allocating memory(memory copy) is not the purposes > for > > > the > > > > passive object > > > > > > > > The reason AudioDecoderConfig/VideoDecoderConfig do that is only because > it > > > the > > > > data won't be huge, using std::vector (memory copy) should be fine. But > > that's > > > > different consideration, from my point of view > > > > > > I'm probably going to insist. The OEM implementation is most likely going to > > > have to memory-copy the std::vector *anyways* (as in your implementation). > > > Assuming that's the case: > > > > > > Arguments against uint8*: our API now passes in a pointer that quickly > becomes > > > invalid. If OEM implementation holds onto copies of the > > AudioConfig/VideoConfig > > > objects (reasonable), they're left with a dangling pointer. > > > > > > Arguments against std::vector: Chromium style guide says we need ctor/dtor > > > implementation in .cc file. > > > > > > To me, that's an easy choice: we should be preferring a better API. > > > > I asked Eric to change the use of std:vector to unit8*. Besides the fact that > > having a C++ data structure would force us to create a separate .cc files > which > > complicates the OEM integration, SoC vendors are traditionally more used to > > C-style APIs than C++ style APIs. Passing in a pointer to a chunk of data and > > its size is a very common (C-style_ API design for platform SDKs or OEM layer > > software. (See the PlayReady SDK APIs and you will find this common pattern.) > > > > What we will need to do is to make sure we document clearly that the > > implementation will need to copy the data explicitly if the data is going to > be > > used beyond the end of the function. > > Also if you look at our existing cma backend code, after receiving the buffers, > we took the address of the buffer data and its size and call the OEM SDK/device > drive APIs (which are C APIs) with them anyway. I agree both that OEM APIs are usually C-structure based and that that reason alone is sufficient to adopt using C-only data types for our public API. I don't agree that we would then need a .cc file. I believe that's only enforced by the clang style plugin, and due to the nature of how our public APIs are to be used it would seem reasonable to me to disable those checks for the public API only. (I could be wrong if the checks also extend to other source files in our tree that include these headers). However, if we feel strongly about using C-style APIs as more comfortable to OEMs, should we mandate not using C++ types in our public APIs? If not, what makes this case different? This seems like a good time to consciously decide what conventions we want to establish for these APIs.
On 2015/05/06 05:24:29, gunsch wrote: > On 2015/05/06 04:17:46, lcwu1 wrote: > > On 2015/05/06 03:57:33, lcwu1 wrote: > > > On 2015/05/06 03:08:44, gunsch wrote: > > > > > > > > > > https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... > > > > File chromecast/public/media/decoder_config.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1074383002/diff/100001/chromecast/public/medi... > > > > chromecast/public/media/decoder_config.h:98: // Size of extra data in > bytes. > > > > On 2015/05/06 03:01:33, erickung1 wrote: > > > > > On 2015/05/06 02:52:35, gunsch wrote: > > > > > > Whoa, I just noticed this change... why did we change from > > > > > std::vector<uint8_t> > > > > > > to uint8_t* + size? That seems like a source of easy mistakes when > OEMs > > > are > > > > > > implementing. > > > > > > > > > > using std:vector will require constructor/destructor implementation in > .cc > > > > > file(Chromium C++ style guild). Although we can consider disabling this > > flag > > > > for > > > > > cast_public_api gyp, allocating memory(memory copy) is not the purposes > > for > > > > the > > > > > passive object > > > > > > > > > > The reason AudioDecoderConfig/VideoDecoderConfig do that is only because > > it > > > > the > > > > > data won't be huge, using std::vector (memory copy) should be fine. But > > > that's > > > > > different consideration, from my point of view > > > > > > > > I'm probably going to insist. The OEM implementation is most likely going > to > > > > have to memory-copy the std::vector *anyways* (as in your implementation). > > > > Assuming that's the case: > > > > > > > > Arguments against uint8*: our API now passes in a pointer that quickly > > becomes > > > > invalid. If OEM implementation holds onto copies of the > > > AudioConfig/VideoConfig > > > > objects (reasonable), they're left with a dangling pointer. > > > > > > > > Arguments against std::vector: Chromium style guide says we need ctor/dtor > > > > implementation in .cc file. > > > > > > > > To me, that's an easy choice: we should be preferring a better API. > > > > > > I asked Eric to change the use of std:vector to unit8*. Besides the fact > that > > > having a C++ data structure would force us to create a separate .cc files > > which > > > complicates the OEM integration, SoC vendors are traditionally more used to > > > C-style APIs than C++ style APIs. Passing in a pointer to a chunk of data > and > > > its size is a very common (C-style_ API design for platform SDKs or OEM > layer > > > software. (See the PlayReady SDK APIs and you will find this common > pattern.) > > > > > > What we will need to do is to make sure we document clearly that the > > > implementation will need to copy the data explicitly if the data is going to > > be > > > used beyond the end of the function. > > > > Also if you look at our existing cma backend code, after receiving the > buffers, > > we took the address of the buffer data and its size and call the OEM > SDK/device > > drive APIs (which are C APIs) with them anyway. > > I agree both that OEM APIs are usually C-structure based and that that reason > alone is sufficient to adopt using C-only data types for our public API. > > I don't agree that we would then need a .cc file. I believe that's only enforced > by the clang style plugin, and due to the nature of how our public APIs are to > be used it would seem reasonable to me to disable those checks for the public > API only. (I could be wrong if the checks also extend to other source files in > our tree that include these headers). I believe it's our source files that include the public headers that the clang plugin complained about. > > However, if we feel strongly about using C-style APIs as more comfortable to > OEMs, should we mandate not using C++ types in our public APIs? If not, what > makes this case different? This seems like a good time to consciously decide > what conventions we want to establish for these APIs. Yes, in general (and in the long run) we should prefer not to use C++ containers/types in our public APIs. In the short term, if it requires a lot of refactoring to not use them and we don't have the bandwidth to do just that, I am OK to leave some of the C++ types in the APIs, but they should exceptions, not norm.
SGTM. This LGTM and I'll start a thread about adjusting those conventions.
The CQ bit was checked by erickung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lcwu@chromium.org, gunsch@google.com Link to the patchset: https://codereview.chromium.org/1074383002/#ps180001 (title: "Add TODO comments and fix unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1074383002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/548865f764227f0755536921d2533f7dcc3de6ae Cr-Commit-Position: refs/heads/master@{#328611} |
