Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14247018:
Implement WebRTC in Chrome for TV (Closed)
|
Created:
7 years, 8 months ago by wonsik Modified:
7 years, 6 months ago Reviewers:
Yaron, Ami GONE FROM CHROMIUM, jam, ycheo (away), acolwell GONE FROM CHROMIUM, qinmin, palmer, dwkang1, scherkus (not reviewing) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org, tv-nikita_google.com, Ami GONE FROM CHROMIUM Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement WebRTC in Chrome for TV
Inject |cricket::WebRtcVideoDecoderFactory| into webrtc for TV case.
The factory will eventually return |RTCVideoDecoderBridgeTvImpl|,
which will communicate with corresponding Google TV API via
|RTCDemuxer|. |RTCDemuxer| is injected to |MediaSourceDelegate| so
that the video frames are fed to hardware.
BUG=233516
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203706
Patch Set 1 #
Total comments: 2
Patch Set 2 : Patch set 8 from https://codereview.chromium.org/14341010/ ; please review diff from this patch set… #Patch Set 3 : Please review delta from patch set 2. #Patch Set 4 : WebRTC impl on Chrome for TV #
Total comments: 24
Patch Set 5 : Timestamp adjustment in RTCVideoDecoderBridgeTv #
Total comments: 29
Patch Set 6 : Addressed some comments #
Total comments: 25
Patch Set 7 : separated refactoring from the CL; resolved nits #Patch Set 8 : Added tests and refined the code #
Total comments: 20
Patch Set 9 : rebase & addressed comments from dwkang@ and ycheo@ #
Total comments: 2
Patch Set 10 : Addressed dwkang's comment #
Total comments: 2
Patch Set 11 : combined KeyHandlingDemxuer w/ KeyHandlingChunkDemuxer #
Total comments: 100
Patch Set 12 : rebase #
Total comments: 4
Patch Set 13 : refactored according to Ami's suggestion #
Total comments: 16
Patch Set 14 : rebase, polished tests, applied Dongwon's comments. #Patch Set 15 : nits #
Total comments: 12
Patch Set 16 : Addressed Dongwon and Yuncheol's comments #
Total comments: 57
Patch Set 17 : rebase & address Aaron's comments #Patch Set 18 : fix bugs #
Total comments: 24
Patch Set 19 : Address Aaron's comments #
Total comments: 12
Patch Set 20 : Address Aaron's comments #Patch Set 21 : rebase #Patch Set 22 : rebase #Patch Set 23 : rebase #Patch Set 24 : rebase #Patch Set 25 : build fix #Messages
Total messages: 52 (0 generated)
Please take a look. Thanks!
https://codereview.chromium.org/14247018/diff/1/content/renderer/media/rtc_vi... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/1/content/renderer/media/rtc_vi... content/renderer/media/rtc_video_decoder_bridge_tv.h:25: class RTCDemuxerProxy : public media::Demuxer { do you have code that's using media::Pipeline and/or media::Audio/VideoDecoder objects? if so, where is it? It's a bit tough to review this change without being to get an idea on how it's intended to be used
https://codereview.chromium.org/14247018/diff/1/content/renderer/media/rtc_vi... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/1/content/renderer/media/rtc_vi... content/renderer/media/rtc_video_decoder_bridge_tv.h:25: class RTCDemuxerProxy : public media::Demuxer { On 2013/04/19 20:19:38, scherkus wrote: > do you have code that's using media::Pipeline and/or media::Audio/VideoDecoder > objects? > > if so, where is it? It's a bit tough to review this change without being to get > an idea on how it's intended to be used Instead of using media::Pipeline, the plan is to inject RTCDemuxerProxy into MediaSourceDelegate in https://codereview.chromium.org/14341010/ as it uses ChunkDemuxer.
PTAL --- please review delta from patch set 2 as it is just a copy from patch set 8 of https://codereview.chromium.org/14341010/ . Thanks!
yfriedman: content/browser/android/* palmer: IPC security ycheo: pretty much everything, especially webkit/media/android/media_source_delegate.cc and related code. everyone else: overall shape of CL. Thanks! (Please ignore Patch Set 1-3.)
https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:80: DCHECK(false) << "Does not support audio."; s/DCHECK(false)/NOTREACHED()/ https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:145: const media::PipelineStatusCB& cb); indentation. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:221: const media::PipelineStatusCB& cb) { indentation. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:288: RTCDemuxer* demuxer_; who owns this? https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:309: if (being_used_) { Remove {} for the single line and apply this through the file. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:403: status_ = kNoInit; How about |being_used_|? Reset the variable or need a sanity check. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_factory_tv.cc:19: if (bridge->AcquireOwnership()) { Remove {} for the single line. https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/medi... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:109: demuxer_.reset(demuxer); move this to the initialization list? https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:518: int access_unit_size = low_latency_mode_ ? 1 : kAccessUnitSize; size_t? https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:661: if (duration_ms > std::numeric_limits<int>::max()) How about moving this logic into KeyHandlingChunkDemuer->GetDurationMs()? https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_tv.cc (right): https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:159: if (audio_renderer_) { Don't need {}.
https://codereview.chromium.org/14247018/diff/31001/content/browser/android/m... File content/browser/android/media_player_manager_android.h (right): https://codereview.chromium.org/14247018/diff/31001/content/browser/android/m... content/browser/android/media_player_manager_android.h:90: void OnInitialize(int player_id, const GURL& url, int flags, why not use MediaPlayerBridge::MediaType Enum instead of int for flags? https://codereview.chromium.org/14247018/diff/31001/media/base/android/media_... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/14247018/diff/31001/media/base/android/media_... media/base/android/media_player_bridge.h:49: FLAG_MEDIA_SOURCE = 1 << 0, add a default flag here for non media source case. https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... webkit/media/android/webmediaplayer_android.cc:619: WebMediaPlayerProxyAndroid* WebMediaPlayerAndroid::proxy() { return proxy_; } move these functions to the header https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_tv.cc (right): https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:159: if (audio_renderer_) { no { }
IPC security LGTM, with nits and a size_t plea. https://codereview.chromium.org/14247018/diff/31001/content/browser/android/m... File content/browser/android/media_player_manager_android.h (right): https://codereview.chromium.org/14247018/diff/31001/content/browser/android/m... content/browser/android/media_player_manager_android.h:90: void OnInitialize(int player_id, const GURL& url, int flags, > why not use MediaPlayerBridge::MediaType Enum instead of int for flags? Yes, I tend to prefer the most specific type possible, too. https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:45: // A singleton object that shared between WebMediaPlayerAndroid and WebRTC NIT: Either "...object that is shared..." or "...object shared..." https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_tv.cc (right): https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:110: unsigned init_data_length, Can any of these unsigned variables be changed to size_t? I forget where we were able to do that and where we weren't. Ultimately, we're going to want to do a size_t refactoring. That's really the right type here.
FYI, on the off-chance you're unaware of it; similar but different work is ongoing in https://codereview.chromium.org/13890012/ where wuchengli@ is hooking up webrtc with HW decode via VDA (which is the path not chosen by the TV project, so this isn't usable there, but I wanted to make sure you're aware of the other effort)
I only did a high-level review, and the CL's shape looks right to me. Some specific comments below. I'm worried about the singleton's lack of explicit safety. https://codereview.chromium.org/14247018/diff/31001/content/content_renderer.... File content/content_renderer.gypi (right): https://codereview.chromium.org/14247018/diff/31001/content/content_renderer.... content/content_renderer.gypi:412: 'renderer/media/rtc_video_decoder_bridge_tv.h', Is any of this new code covered by tests that run automatically somewhere? https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:74: size, gfx::Rect(size), size, NULL, 0, false) { I think many of the comments I made in https://chromiumcodereview.appspot.com/13890012/ (on the bridge code) probably also apply here. You might want to take a stroll through that. https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:45: // A singleton object that shared between WebMediaPlayerAndroid and WebRTC It's a singleton but there are no locks I can see. What happens when two decoders are requested in the same page (or different pages in the same renderer)? https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:62: virtual bool RegisterDemuxer(RTCDemuxer* demuxer) = 0; Please doco the usage of this and the *Ownership() methods below. https://codereview.chromium.org/14247018/diff/31001/media/base/android/media_... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/14247018/diff/31001/media/base/android/media_... media/base/android/media_player_bridge.h:50: FLAG_LOW_LATENCY = 1 << 1, Neither of these names is meaningful to me. Does MEDIA_SOURCE refer to the W3C MediaSource Extensions spec, or to the GoogleTV MediaSource API? What does LOW_LATENCY refer to? (why would anyone want _high_ latency? ;)) Needs fuller commentary (since these aren't described in the android doco linked from the class comment) and hopefully a more specific name for MEDIA_SOURCE (off the top of my head FLAG_MEDIA_SOURCE_EXTENSIONS and FLAG_GOOGLETV_MEDIA_SOURCE_API make more sense to me, but that's not to say there aren't better choices) https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/medi... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:75: return WebKit::WebMediaPlayer::MediaKeyExceptionKeySystemNotSupported; lol more like Key_NOT_HandlingDemuxer! :) https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:662: duration_ms = std::numeric_limits<int>::max(); This seems pretty fishy.
Addressed most of comments. Not yet strolled thoroughly through https://codereview.chromium.org/13890012/ and missing a test. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:80: DCHECK(false) << "Does not support audio."; On 2013/04/29 13:18:13, Yuncheol Heo wrote: > s/DCHECK(false)/NOTREACHED()/ Done. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:145: const media::PipelineStatusCB& cb); On 2013/04/29 13:18:13, Yuncheol Heo wrote: > indentation. Done. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:221: const media::PipelineStatusCB& cb) { On 2013/04/29 13:18:13, Yuncheol Heo wrote: > indentation. Done. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:288: RTCDemuxer* demuxer_; On 2013/04/29 13:18:13, Yuncheol Heo wrote: > who owns this? RTCVideoDecoderBridgeTvImpl owns it now. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:309: if (being_used_) { On 2013/04/29 13:18:13, Yuncheol Heo wrote: > Remove {} for the single line and apply this through the file. Done. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:403: status_ = kNoInit; On 2013/04/29 13:18:13, Yuncheol Heo wrote: > How about |being_used_|? > Reset the variable or need a sanity check. demuxer_'s life cycle changed. being_used_ is about ownership and not related to this method. https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/24001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_factory_tv.cc:19: if (bridge->AcquireOwnership()) { On 2013/04/29 13:18:13, Yuncheol Heo wrote: > Remove {} for the single line. Done. https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/medi... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:109: demuxer_.reset(demuxer); On 2013/04/29 13:18:13, Yuncheol Heo wrote: > move this to the initialization list? Done. https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:518: int access_unit_size = low_latency_mode_ ? 1 : kAccessUnitSize; On 2013/04/29 13:18:13, Yuncheol Heo wrote: > size_t? Done. https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:661: if (duration_ms > std::numeric_limits<int>::max()) On 2013/04/29 13:18:13, Yuncheol Heo wrote: > How about moving this logic into KeyHandlingChunkDemuer->GetDurationMs()? Done. https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_tv.cc (right): https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/04/29 13:18:13, Yuncheol Heo wrote: > 2013 Done. https://codereview.chromium.org/14247018/diff/24001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:159: if (audio_renderer_) { On 2013/04/29 13:18:13, Yuncheol Heo wrote: > Don't need {}. Done. https://codereview.chromium.org/14247018/diff/31001/content/browser/android/m... File content/browser/android/media_player_manager_android.h (right): https://codereview.chromium.org/14247018/diff/31001/content/browser/android/m... content/browser/android/media_player_manager_android.h:90: void OnInitialize(int player_id, const GURL& url, int flags, On 2013/04/29 18:58:44, Chromium Palmer wrote: > > why not use MediaPlayerBridge::MediaType Enum instead of int for flags? > > Yes, I tend to prefer the most specific type possible, too. Done. https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:45: // A singleton object that shared between WebMediaPlayerAndroid and WebRTC On 2013/04/29 18:58:44, Chromium Palmer wrote: > NIT: Either "...object that is shared..." or "...object shared..." Done. https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:45: // A singleton object that shared between WebMediaPlayerAndroid and WebRTC On 2013/04/29 22:11:09, Ami Fischman wrote: > It's a singleton but there are no locks I can see. What happens when two > decoders are requested in the same page (or different pages in the same > renderer)? Added locking. https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:62: virtual bool RegisterDemuxer(RTCDemuxer* demuxer) = 0; On 2013/04/29 22:11:09, Ami Fischman wrote: > Please doco the usage of this and the *Ownership() methods below. Done. https://codereview.chromium.org/14247018/diff/31001/media/base/android/media_... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/14247018/diff/31001/media/base/android/media_... media/base/android/media_player_bridge.h:49: FLAG_MEDIA_SOURCE = 1 << 0, On 2013/04/29 17:31:49, qinmin wrote: > add a default flag here for non media source case. Done. https://codereview.chromium.org/14247018/diff/31001/media/base/android/media_... media/base/android/media_player_bridge.h:50: FLAG_LOW_LATENCY = 1 << 1, On 2013/04/29 22:11:09, Ami Fischman wrote: > Neither of these names is meaningful to me. > Does MEDIA_SOURCE refer to the W3C MediaSource Extensions spec, or to the > GoogleTV MediaSource API? > What does LOW_LATENCY refer to? (why would anyone want _high_ latency? ;)) > Needs fuller commentary (since these aren't described in the android doco linked > from the class comment) and hopefully a more specific name for MEDIA_SOURCE (off > the top of my head FLAG_MEDIA_SOURCE_EXTENSIONS and > FLAG_GOOGLETV_MEDIA_SOURCE_API make more sense to me, but that's not to say > there aren't better choices) Improved names. https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/medi... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:75: return WebKit::WebMediaPlayer::MediaKeyExceptionKeySystemNotSupported; On 2013/04/29 22:11:09, Ami Fischman wrote: > lol more like Key_NOT_HandlingDemuxer! :) Right, just interfaces and stub. I guess something like |KeyHandlingDemxuerStub| may make more sense --- what do you think? :) https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:662: duration_ms = std::numeric_limits<int>::max(); On 2013/04/29 22:11:09, Ami Fischman wrote: > This seems pretty fishy. Oops. Moved to the right place. https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... webkit/media/android/webmediaplayer_android.cc:619: WebMediaPlayerProxyAndroid* WebMediaPlayerAndroid::proxy() { return proxy_; } On 2013/04/29 17:31:49, qinmin wrote: > move these functions to the header Done. https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_tv.cc (right): https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/04/29 17:31:49, qinmin wrote: > 2013 Done. https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:110: unsigned init_data_length, On 2013/04/29 18:58:44, Chromium Palmer wrote: > Can any of these unsigned variables be changed to size_t? I forget where we were > able to do that and where we weren't. > > Ultimately, we're going to want to do a size_t refactoring. That's really the > right type here. Well these method signatures come all the way from WebKit :) https://codereview.chromium.org/14247018/diff/31001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:159: if (audio_renderer_) { On 2013/04/29 17:31:49, qinmin wrote: > no { } Done.
https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... webkit/media/android/webmediaplayer_android.cc:167: if (media_source_delegate_) If the goal of most of the changes in this file are to move the GOOGLE_TV specific functionality out of here and into webmediaplayer_tv, then I'd prefer to see a separate CL that only contains that refactoring. Doing so will make reviewing the refactoring and this new functionality much easier.
https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:45: // Returns a demuxer object if this object is not owned; otherwise, NULL. Is the "otherwise, NULL" always an indication of programming error? If yes then the comment can be rephrased more strictly, and a DCHECK added to the impl. https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:45: // Returns a demuxer object if this object is not owned; otherwise, NULL. Also returns NULL the second time it's called when called twice in a row. Generally the API here is pretty gnarly, with *Ownership(==being_used_) and CreateDemuxer (*generation_) being non-orthogonal. Does it not work to drop the *Ownership methods and replace CreateDemuxer with CreateAndAcquireDemuxer() which returns NULL iff it has been called previously without an intervening ReleaseDemuxer() (new call)? That way you can drop being_used_ and generation_/demuxer_generation_ (and have ReleaseDemuxer() simply be demuxer_.reset()). https://codereview.chromium.org/14247018/diff/59001/media/base/android/media_... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/14247018/diff/59001/media/base/android/media_... media/base/android/media_player_bridge.h:47: // Types of media that this object will play. Wow, I *seriously* did not understand what you were doing here, before. WDYT of enum MediaSource { MEDIA_SOURCE_URL, MEDIA_SOURCE_MSE, // W3C Media Source Extensions. MEDIA_SOURCE_STREAM, // W3C Media Stream, e.g. getUserMedia(). }; ? https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/medi... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:323: if (duration_ms > std::numeric_limits<int>::max()) this is still pretty fishy...
https://codereview.chromium.org/14247018/diff/59001/content/content_renderer.... File content/content_renderer.gypi (right): https://codereview.chromium.org/14247018/diff/59001/content/content_renderer.... content/content_renderer.gypi:413: 'renderer/media/rtc_video_decoder_factory_tv.cc', Sort these files alphabetically. https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:56: void RunDoneCallback(); private? https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:378: if (demuxer_ && demuxer_generation_ == generation_) You may want to add some warning message if the generation is different. https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/we... File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/we... content/renderer/media/webmediaplayer_proxy_impl_android.cc:216: static_cast<webkit_media::WebMediaPlayerTv*>( dynamic_cast? https://codereview.chromium.org/14247018/diff/59001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/render_v... content/renderer/render_view_impl.cc:2743: return NULL; Could you add some error message for this? https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_tv.cc (right): https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:148: // |media_source_delegate_| is owned, so Unretained() is safe here. Move this comment in front of L146. https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:151: media_type = MediaPlayerBridge::MEDIA_TYPE_MEDIA_SOURCE_EXTENSIONS; How about move this line at the beginning of the block? It can have some documentation effects. https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:162: media_type = MediaPlayerBridge::MEDIA_TYPE_MEDIA_STREAMS; Ditto.
I need to think about how to protect the singleton explicitly and cleanly. https://codereview.chromium.org/14247018/diff/59001/content/content_renderer.... File content/content_renderer.gypi (right): https://codereview.chromium.org/14247018/diff/59001/content/content_renderer.... content/content_renderer.gypi:413: 'renderer/media/rtc_video_decoder_factory_tv.cc', On 2013/05/02 02:45:21, Yuncheol Heo wrote: > Sort these files alphabetically. Done. https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:56: void RunDoneCallback(); On 2013/05/02 02:45:21, Yuncheol Heo wrote: > private? Done. https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/we... File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/we... content/renderer/media/webmediaplayer_proxy_impl_android.cc:216: static_cast<webkit_media::WebMediaPlayerTv*>( On 2013/05/02 02:45:21, Yuncheol Heo wrote: > dynamic_cast? Not permitted (-fno-rtti), and not applicable after separating refactoring. https://codereview.chromium.org/14247018/diff/59001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/render_v... content/renderer/render_view_impl.cc:2743: return NULL; On 2013/05/02 02:45:21, Yuncheol Heo wrote: > Could you add some error message for this? Done. https://codereview.chromium.org/14247018/diff/59001/media/base/android/media_... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/14247018/diff/59001/media/base/android/media_... media/base/android/media_player_bridge.h:47: // Types of media that this object will play. On 2013/05/01 22:01:37, Ami Fischman wrote: > Wow, I *seriously* did not understand what you were doing here, before. My bad -- I confused implementation details with the media type (W3C Media Source Extension happened to be implemented by Google TV Media Source API). > WDYT of > enum MediaSource { > MEDIA_SOURCE_URL, > MEDIA_SOURCE_MSE, // W3C Media Source Extensions. > MEDIA_SOURCE_STREAM, // W3C Media Stream, e.g. getUserMedia(). > }; > ? Done. https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/medi... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:323: if (duration_ms > std::numeric_limits<int>::max()) On 2013/05/01 22:01:37, Ami Fischman wrote: > this is still pretty fishy... Logged warning. https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... webkit/media/android/webmediaplayer_android.cc:167: if (media_source_delegate_) On 2013/05/01 14:34:28, acolwell wrote: > If the goal of most of the changes in this file are to move the GOOGLE_TV > specific functionality out of here and into webmediaplayer_tv, then I'd prefer > to see a separate CL that only contains that refactoring. Doing so will make > reviewing the refactoring and this new functionality much easier. Done. https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... File webkit/media/android/webmediaplayer_tv.cc (right): https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:148: // |media_source_delegate_| is owned, so Unretained() is safe here. On 2013/05/02 02:45:21, Yuncheol Heo wrote: > Move this comment in front of L146. Done. https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:151: media_type = MediaPlayerBridge::MEDIA_TYPE_MEDIA_SOURCE_EXTENSIONS; On 2013/05/02 02:45:21, Yuncheol Heo wrote: > How about move this line at the beginning of the block? > It can have some documentation effects. Done. https://codereview.chromium.org/14247018/diff/59001/webkit/media/android/webm... webkit/media/android/webmediaplayer_tv.cc:162: media_type = MediaPlayerBridge::MEDIA_TYPE_MEDIA_STREAMS; On 2013/05/02 02:45:21, Yuncheol Heo wrote: > Ditto. Done.
PTAL https://codereview.chromium.org/14247018/diff/31001/content/content_renderer.... File content/content_renderer.gypi (right): https://codereview.chromium.org/14247018/diff/31001/content/content_renderer.... content/content_renderer.gypi:412: 'renderer/media/rtc_video_decoder_bridge_tv.h', On 2013/04/29 22:11:09, Ami Fischman wrote: > Is any of this new code covered by tests that run automatically somewhere? Added a test. https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/31001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:74: size, gfx::Rect(size), size, NULL, 0, false) { On 2013/04/29 22:11:09, Ami Fischman wrote: > I think many of the comments I made in > https://chromiumcodereview.appspot.com/13890012/ (on the bridge code) probably > also apply here. You might want to take a stroll through that. Went through the review and applied a few. https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:378: if (demuxer_ && demuxer_generation_ == generation_) On 2013/05/02 02:45:21, Yuncheol Heo wrote: > You may want to add some warning message if the generation is different. N/A https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/59001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:45: // Returns a demuxer object if this object is not owned; otherwise, NULL. On 2013/05/01 22:01:37, Ami Fischman wrote: > Also returns NULL the second time it's called when called twice in a row. > Generally the API here is pretty gnarly, with *Ownership(==being_used_) and > CreateDemuxer (*generation_) being non-orthogonal. > > Does it not work to drop the *Ownership methods and replace CreateDemuxer with > CreateAndAcquireDemuxer() which returns NULL iff it has been called previously > without an intervening ReleaseDemuxer() (new call)? > > That way you can drop being_used_ and generation_/demuxer_generation_ (and have > ReleaseDemuxer() simply be demuxer_.reset()). This singleton is mainly used in two different part --- one is peer connection through VDF and the other is webmediaplayer_android where it accesses media::Demuxer object. Since this is a singleton, it should make sure only one peer connection and one webmediaplayer_android accesses the object. In addition, the two must be the ones working together. I used MediaStreamDependencyFactory to find out which is working with which. Perhaps I can use RenderThreadImpl. Please take a look again.
+dwkang
LGTM modulo some minor comments. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:58: struct DecoderBuffer { This name is a little confused with media::DecoderBuffer. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:64: scoped_refptr<media::DecoderBuffer> decoder_buffer_; We don't add '_' suffix at the data members in structs. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:291: const webrtc::RTPFragmentationHeader* /* fragmentation */, Why do you wrap this /* */? https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:39: // RTCVideoDecoderBridgeTv::Get()->ReleaseOwnership(); How about adding DestroyDemuxer() into the example? https://codereview.chromium.org/14247018/diff/83001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/render_v... content/renderer/render_view_impl.cc:2757: web_media_player_android->SetDestroyDemuxerCB( Should we run this code only if it is the media stream? https://codereview.chromium.org/14247018/diff/83001/webkit/media/android/medi... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/83001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:423: WebKit::WebFrame * frame, Attach '*' on the type.
Thanks a lot for caring this work. Looking good. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:66: gfx::Size size_; Could you add a doc for this variable? (when this is empty.) https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:71: std::queue<DecoderBuffer> buffer_queue_; Member variables need to be protected because QueueBuffer() will be called in the webrtc thread. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:493: callback->Decoded(dummy_video_frame); Could you add some comments about the background of this? https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_factory_tv.h (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_factory_tv.h:23: class CONTENT_EXPORT RTCVideoDecoderFactoryTv Any reason to have a separate factory for TV? (We can save 2 files if we have ifdef in factory implementation rather than media_stream_dependency_factory.)
PTAL https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:58: struct DecoderBuffer { On 2013/05/06 05:45:34, Yuncheol Heo wrote: > This name is a little confused with media::DecoderBuffer. Done --- Used BufferEntry https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:64: scoped_refptr<media::DecoderBuffer> decoder_buffer_; On 2013/05/06 05:45:34, Yuncheol Heo wrote: > We don't add '_' suffix at the data members in structs. Done. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:66: gfx::Size size_; On 2013/05/06 09:43:38, dwkang1 wrote: > Could you add a doc for this variable? (when this is empty.) Done. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:71: std::queue<DecoderBuffer> buffer_queue_; On 2013/05/06 09:43:38, dwkang1 wrote: > Member variables need to be protected because QueueBuffer() will be called in > the webrtc thread. Done. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:291: const webrtc::RTPFragmentationHeader* /* fragmentation */, On 2013/05/06 05:45:34, Yuncheol Heo wrote: > Why do you wrap this /* */? I tried to indicate that this argument is not used, but after second thought, it seems useless. Unwrapped. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.cc:493: callback->Decoded(dummy_video_frame); On 2013/05/06 09:43:38, dwkang1 wrote: > Could you add some comments about the background of this? Done. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_bridge_tv.h:39: // RTCVideoDecoderBridgeTv::Get()->ReleaseOwnership(); On 2013/05/06 05:45:34, Yuncheol Heo wrote: > How about adding DestroyDemuxer() into the example? Oops. I forgot to modify the example. Done. https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... File content/renderer/media/rtc_video_decoder_factory_tv.h (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/media/rt... content/renderer/media/rtc_video_decoder_factory_tv.h:23: class CONTENT_EXPORT RTCVideoDecoderFactoryTv On 2013/05/06 09:43:38, dwkang1 wrote: > Any reason to have a separate factory for TV? (We can save 2 files if we have > ifdef in factory implementation rather than media_stream_dependency_factory.) This factory takes pointer to MediaStreamDependencyFactory in constructor to use as "owenership tag" which I don't think is common across all platform. https://codereview.chromium.org/14247018/diff/83001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14247018/diff/83001/content/renderer/render_v... content/renderer/render_view_impl.cc:2757: web_media_player_android->SetDestroyDemuxerCB( On 2013/05/06 05:45:34, Yuncheol Heo wrote: > Should we run this code only if it is the media stream? Done. https://codereview.chromium.org/14247018/diff/83001/webkit/media/android/medi... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/83001/webkit/media/android/medi... webkit/media/android/media_source_delegate.cc:423: WebKit::WebFrame * frame, On 2013/05/06 05:45:34, Yuncheol Heo wrote: > Attach '*' on the type. Done.
LGTM modulo a minor comment. https://chromiumcodereview.appspot.com/14247018/diff/100001/content/renderer/... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://chromiumcodereview.appspot.com/14247018/diff/100001/content/renderer/... content/renderer/media/rtc_video_decoder_bridge_tv.cc:135: void RTCDemuxerStream::RunReadCallback() { How about adding "_Locked" suffix?
https://codereview.chromium.org/14247018/diff/100001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/100001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:135: void RTCDemuxerStream::RunReadCallback() { On 2013/05/07 00:51:18, dwkang1 wrote: > How about adding "_Locked" suffix? Done.
https://codereview.chromium.org/14247018/diff/115001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/115001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:66: class KeyHandlingDemuxer : public media::Demuxer { this file defines both a base class and an inherited class. Can we combine them? which class other than KeyHandlingChunkDemuxer will inherit KeyHandlingDemuxer?
PTAL https://codereview.chromium.org/14247018/diff/115001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/115001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:66: class KeyHandlingDemuxer : public media::Demuxer { On 2013/05/07 02:41:24, qinmin wrote: > this file defines both a base class and an inherited class. Can we combine them? > which class other than KeyHandlingChunkDemuxer will inherit KeyHandlingDemuxer? Done.
media/base/android and webkit/media/android lgtm On 2013/05/07 07:12:26, wonsik wrote: > PTAL > > https://codereview.chromium.org/14247018/diff/115001/webkit/media/android/med... > File webkit/media/android/media_source_delegate.cc (right): > > https://codereview.chromium.org/14247018/diff/115001/webkit/media/android/med... > webkit/media/android/media_source_delegate.cc:66: class KeyHandlingDemuxer : > public media::Demuxer { > On 2013/05/07 02:41:24, qinmin wrote: > > this file defines both a base class and an inherited class. Can we combine > them? > > which class other than KeyHandlingChunkDemuxer will inherit > KeyHandlingDemuxer? > > Done.
Ami & Aaron: Could you have another look at the overall shape & the code itself? Thanks!
content/browser/android lgtm
Wonsik: I discussed w/ acolwell@ and he agreed to take over this review from me (for videostack side) after I send this set of comments (starting with patchset 13; I reviewed patchset #12). Please feel free to ping me directly if you have any questions about what I'm asking/suggesting here. https://codereview.chromium.org/14247018/diff/118001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/14247018/diff/118001/content/browser/android/... content/browser/android/content_view_core_impl.cc:19: #include "content/browser/android/media_player_manager_impl.h" needed? https://codereview.chromium.org/14247018/diff/118001/content/content_renderer... File content/content_renderer.gypi (right): https://codereview.chromium.org/14247018/diff/118001/content/content_renderer... content/content_renderer.gypi:417: ['enable_webrtc==1 and google_tv==1', { Do you have numbers quantifying the benefit of this yet? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:24: // Helper function that makes sure |read_cb| runs on |message_loop_proxy|. I think you converted all callsites to media::BindToLoop but forgot to delete this. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:40: namespace { why? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:70: gfx::Size size; new_size ? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:80: media::AudioDecoderConfig dummy_audio_decoder_config_; unnecessary; can simply return media::AudioDecoderConfig(); in audio_decoder_config(), below. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:86: : video_decoder_config_( indent https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:90: size, gfx::Rect(size), size, NULL, 0, false) { This assumes that width & height are macroblock-aligned, so the coded stream has no padding, and there is no cropping. None of those are guaranteed by the webrtc spec; why is it ok to manufacture these values here? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:136: if (!read_cb_.is_null() && !buffer_queue_.empty()) { If you reverse the test you can de-indent the rest of the function. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:142: // coded_size == visible_rect == natural_size here. See comment elsewhere; this seems wrong since you don't get to dictate the contents of the bitstream - it may well have padding that you don't want to render. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:162: class RTCDemuxerProxy : public base::RefCountedThreadSafe<RTCDemuxerProxy> { Why does need to be separate from RTCDemuxer? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:207: if (!loop_proxy_->BelongsToCurrentThread()) { Is the trampolining here and elsewhere in this file really necessary? Can they be replaced with unconditional PostTasks or DCHECKs (if the caller is always on the right thread already). https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:214: if (!init_cb_.is_null()) It seems strange to me to be triggering init-done off the UpdateSize event. How is error handled? (i.e. if CreateDemuxer early-returns NULL, what notifies the caller of Initialize()?) https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:252: friend class RTCVideoDecoderBridgeTv; Why does this make sense? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:273: return base::TimeDelta(); what happens if one joins the middle of a stream? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:288: class RTCVideoDecoderBridgeTvImpl : public RTCVideoDecoderBridgeTv { Is it important that this Impl be a separate class from the non-Impl version? What does that gain (other than boilerplate duplication) https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:332: }; bool is_initialized; instead? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:339: const MediaStreamDependencyFactory* demuxer_tag_; is this only necessary because you don't trust the owner to call CreateDemuxer exactly once? That seems strange. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:344: // Only used by decoder thread. Please DCHECK running on the correct thread at the top of methods that touch this state. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:347: WebRtc_Word64 timestamp_offset_; Use "millis" in the name if this is storing millis. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:414: base::AutoLock lock(lock_); putting this at the top of the function is more robust to future changes. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:420: if (demuxer_) How can this fail? (isn't it a programming error to call InitDecode() before CreateDemuxer()?) https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:430: const webrtc::CodecSpecificInfo* /* codecSpecificInfo */, do you not need to assert that these aren't specified, since you can't deal with them? Or are they optional output params? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:437: base::AutoLock lock(lock_); ditto move lock to top of function unless there's a good reason not to (I'll stop commenting on this now; please make a pass) https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:444: if (!demuxer_) Ditto l.420 this seems like a programming error. Perhaps this is coded with a view that you don't know who's creating the demuxer first, the WMPA or webrtc (which is fed by the idea that MSDF needs to track ownership agnostically). But I think you can rationalize this significantly by enforcing a setup order, which will (as a side-effect) make code like this unnecessary. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:448: // If the first frame is not the key frame, return an error to request a key s/the key/a key/ https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:453: // Google TV expects timestamp from 0. This is a cryptic comment unless one sees the subtraction at l.469. Can you make it clearer? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:460: // Only key frame has the size. This comment is at odds with the lines of code preceding it. If the comment is correct then the test for keyframe is unnecessary (and the comment should probably precede the if test) https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:465: buffer = media::DecoderBuffer::CopyFrom( move assignment to declaration at previous line https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:466: inputImage._buffer, inputImage._length); is it necessary to make a copy here? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:476: new_size); Why not simply pass new_size into the call at l.478 below? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:511: dummy_video_frame.CreateEmptyFrame(2, 1, 2, 1, 1); These are some magical magic values. Doco meaning? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:41: // // NOTE: the order is not significant. Then why not make ReleaseOwnership implicitly call DestroyDemuxer? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:54: const MediaStreamDependencyFactory* media_stream_dependency_factory, I'm still pretty unhappy about this business with the singleton that magically knows who it wants to talk to based on an address (therefore brittle to delete followed by new reusing the same address). Singletons are bad enough, but this pattern unnecessarily bleeds that singletonness into multiple classes' public APIs. ISTM this ought to work by having RVI::createMediaPlayer (or WMPA's ctor) acquire the singleton factory (or NULL if unavailable) and then pass it through to the downstream deps that need it. It'll probably mean widening some of the APIs (like RenderThreadImpl::GetMediaStreamDependencyFactory()) with an additional parameter, but that seems much more reasonable/readable/robust to me. Am I missing something? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:32: void ReadCallback(media::DemuxerStream::Status status, intentionally ignoring |status|? https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:38: void ExpectEqualsAndSignal(int32_t expected_result, int32_t actual_result) { s/_result//g https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:38: void ExpectEqualsAndSignal(int32_t expected_result, int32_t actual_result) { s/_t//g https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:107: bridge_->AcquireOwnership(&owner_factory_); What if this fails? (I'd say CHECK the return value, but that prevents unrelated tests from running on bots so is frowned upon; instead, ASSERT_TRUE here and ASSERT no errors in the caller; e.g. https://code.google.com/p/googletest/wiki/AdvancedGuide?#Asserting_on_Subrout...) https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:120: decoder_thread_event_.Wait(); here and elsewhere, if expectations have already failed, you probably don't want to keep forging on (see previous comment's link) https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:138: EXPECT_EQ(size_, video_stream_->video_decoder_config().natural_size()); This would be a fantastic place to test support for visible/crop rect, coded size, and visible size. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:181: EXPECT_EQ(static_cast<media::Demuxer*>(NULL), FWIW, here and elsewhere, instead of static_cast<LongType::Name*>(NULL) you can simply test for truth: EXPECT_FALSE(bridge_->CreateDemuxer(&non_owner_factory_, message_loop_proxy_)); https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:194: EXPECT_TRUE(bridge_->AcquireOwnership(&owner_factory_)); here and elsewhere, always release. Since you're using a singleton it would be a good idea to run this suite with gtest's randomization & repetition support to make sure the tests all clean up after themselves. (e.g. I believe that if this and the first TEST_F above were swapped in order, the latter would fail). https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.h (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.h:23: class CONTENT_EXPORT RTCVideoDecoderFactoryTv AFAICT this is used only in one place: the mediastreamdependencyfactory cc file. Why not move the entire class into that .cc file? https://codereview.chromium.org/14247018/diff/118001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/render_... content/renderer/render_view_impl.cc:2696: #if defined(ENABLE_WEBRTC) && !defined(GOOGLE_TV) The conditional compile & runtime behavior in this function is pretty gnarly... https://codereview.chromium.org/14247018/diff/118001/content/renderer/render_... content/renderer/render_view_impl.cc:2756: #if defined(GOOGLE_TV) shouldn't this be checking for WEBRTC? https://codereview.chromium.org/14247018/diff/118001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/14247018/diff/118001/media/base/android/media... media/base/android/media_player_bridge.cc:49: LOG_IF(WARNING, media_source != MEDIA_SOURCE_URL) I think this is a programming error, in which case this should be DCHECK_EQ(media_source, MEDIA_SOURCE_URL) << "Unsupported config!"; https://codereview.chromium.org/14247018/diff/118001/media/base/android/media... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/14247018/diff/118001/media/base/android/media... media/base/android/media_player_bridge.h:47: enum MediaSource { MediaSourceType? (otherwise, e.g. void OnInitialize(..., media::MediaPlayerBridge::MediaSource media_source, ...) looks like it's MSE or GTV MediaSource API, not an enum that specifies the source of the media data) https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:68: typedef base::Callback<void(const std::string& key_system)> NotifyDemuxerCB; This name doesn't tell me what this is going to be doing... https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:78: NotifyDemuxerCB notify_demuxer_cb); CB's are usually passed by const-ref https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... File webkit/media/android/media_source_delegate.h (right): https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... webkit/media/android/media_source_delegate.h:54: void InitializeMediaStream(media::Demuxer* demuxer, Is it easy to see why this variant wouldn't want a media_log? https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... webkit/media/android/media_source_delegate.h:130: bool is_media_stream_; Should this not be a MediaSourceType? In fact since the only place it's used is in determining the access_unit_size, ISTM it would be better to simply store access_unit_size_ as a member, initialized appropriately in the InitializeMedia{Source,Stream}() methods. https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/web... File webkit/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:108: // The |audio_renderer_| can be shared by multiple remote streams, and Does this not also apply to multiple local streams? https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:147: base::Unretained(this)))); Comment about Unretained() from above applies here as well. In fact, this and the statement at l.135 are identical, so IWBN to extract & dedup them. https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:188: if (audio_renderer_ && paused()) Can these changes to the audio_renderer_'s play/pause/stop behavior be pulled out into a (much smaller) CL? https://codereview.chromium.org/14247018/diff/124001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/124001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:55: const scoped_refptr<base::MessageLoopProxy>& message_loop) = 0; What is |message_loop|? is it for callback dispatch, or what? I believe all callsites just use MessageLoopProxy::current(), in which case it's silly to pass this explicitly through so many layers, and instead should be replaced by doco that explains the guarantee that callbacks get fired on the calling thread (for whichever class/method actually cares). https://codereview.chromium.org/14247018/diff/124001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/124001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:34: // The size of the access unit to transfer in an IPC. This is silently ignored for mediastreams. The name should reflect the only case it's applied to.
Gone through a refactoring. Aaron, could you please take a look? There's a caveat: the test is lagging behind. https://codereview.chromium.org/14247018/diff/118001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/14247018/diff/118001/content/browser/android/... content/browser/android/content_view_core_impl.cc:19: #include "content/browser/android/media_player_manager_impl.h" On 2013/05/08 20:26:44, Ami Fischman wrote: > needed? For Google TV. Already merged in master. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:24: // Helper function that makes sure |read_cb| runs on |message_loop_proxy|. On 2013/05/08 20:26:44, Ami Fischman wrote: > I think you converted all callsites to media::BindToLoop but forgot to delete > this. Removed. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:40: namespace { On 2013/05/08 20:26:44, Ami Fischman wrote: > why? N/A after refactoring https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:70: gfx::Size size; On 2013/05/08 20:26:44, Ami Fischman wrote: > new_size ? Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:80: media::AudioDecoderConfig dummy_audio_decoder_config_; On 2013/05/08 20:26:44, Ami Fischman wrote: > unnecessary; can simply > return media::AudioDecoderConfig(); > in audio_decoder_config(), below. Because audio_decoder_config() returns const reference, it's not quite possible. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:86: : video_decoder_config_( On 2013/05/08 20:26:44, Ami Fischman wrote: > indent Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:136: if (!read_cb_.is_null() && !buffer_queue_.empty()) { On 2013/05/08 20:26:44, Ami Fischman wrote: > If you reverse the test you can de-indent the rest of the function. Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:162: class RTCDemuxerProxy : public base::RefCountedThreadSafe<RTCDemuxerProxy> { On 2013/05/08 20:26:44, Ami Fischman wrote: > Why does need to be separate from RTCDemuxer? Removed during refactoring. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:207: if (!loop_proxy_->BelongsToCurrentThread()) { On 2013/05/08 20:26:44, Ami Fischman wrote: > Is the trampolining here and elsewhere in this file really necessary? Can they > be replaced with unconditional PostTasks or DCHECKs (if the caller is always on > the right thread already). N/A after refactoring. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:214: if (!init_cb_.is_null()) On 2013/05/08 20:26:44, Ami Fischman wrote: > It seems strange to me to be triggering init-done off the UpdateSize event. How > is error handled? (i.e. if CreateDemuxer early-returns NULL, what notifies the > caller of Initialize()?) N/A after refactoring. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:252: friend class RTCVideoDecoderBridgeTv; On 2013/05/08 20:26:44, Ami Fischman wrote: > Why does this make sense? Removed https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:288: class RTCVideoDecoderBridgeTvImpl : public RTCVideoDecoderBridgeTv { On 2013/05/08 20:26:44, Ami Fischman wrote: > Is it important that this Impl be a separate class from the non-Impl version? > What does that gain (other than boilerplate duplication) Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:332: }; On 2013/05/08 20:26:44, Ami Fischman wrote: > bool is_initialized; > instead? Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:339: const MediaStreamDependencyFactory* demuxer_tag_; On 2013/05/08 20:26:44, Ami Fischman wrote: > is this only necessary because you don't trust the owner to call CreateDemuxer > exactly once? That seems strange. Removed. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:344: // Only used by decoder thread. On 2013/05/08 20:26:44, Ami Fischman wrote: > Please DCHECK running on the correct thread at the top of methods that touch > this state. N/A after refactoring https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:347: WebRtc_Word64 timestamp_offset_; On 2013/05/08 20:26:44, Ami Fischman wrote: > Use "millis" in the name if this is storing millis. Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:414: base::AutoLock lock(lock_); On 2013/05/08 20:26:44, Ami Fischman wrote: > putting this at the top of the function is more robust to future changes. N/A after refactoring; RTCVideoDecoderBridgeTv is only accessed by the decoder thread. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:420: if (demuxer_) On 2013/05/08 20:26:44, Ami Fischman wrote: > How can this fail? (isn't it a programming error to call InitDecode() before > CreateDemuxer()?) N/A after refactoring https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:437: base::AutoLock lock(lock_); On 2013/05/08 20:26:44, Ami Fischman wrote: > ditto move lock to top of function unless there's a good reason not to (I'll > stop commenting on this now; please make a pass) N/A after refactoring https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:444: if (!demuxer_) On 2013/05/08 20:26:44, Ami Fischman wrote: > Ditto l.420 this seems like a programming error. > > Perhaps this is coded with a view that you don't know who's creating the demuxer > first, the WMPA or webrtc (which is fed by the idea that MSDF needs to track > ownership agnostically). But I think you can rationalize this significantly by > enforcing a setup order, which will (as a side-effect) make code like this > unnecessary. N/A after refactoring https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:448: // If the first frame is not the key frame, return an error to request a key On 2013/05/08 20:26:44, Ami Fischman wrote: > s/the key/a key/ Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:453: // Google TV expects timestamp from 0. On 2013/05/08 20:26:44, Ami Fischman wrote: > This is a cryptic comment unless one sees the subtraction at l.469. Can you > make it clearer? Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:460: // Only key frame has the size. On 2013/05/08 20:26:44, Ami Fischman wrote: > This comment is at odds with the lines of code preceding it. > If the comment is correct then the test for keyframe is unnecessary (and the > comment should probably precede the if test) Modified the comment. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:465: buffer = media::DecoderBuffer::CopyFrom( On 2013/05/08 20:26:44, Ami Fischman wrote: > move assignment to declaration at previous line Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:466: inputImage._buffer, inputImage._length); On 2013/05/08 20:26:44, Ami Fischman wrote: > is it necessary to make a copy here? Yes since inputImage's lifetime is not guaranteed after this method returns. Left a comment. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:476: new_size); On 2013/05/08 20:26:44, Ami Fischman wrote: > Why not simply pass new_size into the call at l.478 below? Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:511: dummy_video_frame.CreateEmptyFrame(2, 1, 2, 1, 1); On 2013/05/08 20:26:44, Ami Fischman wrote: > These are some magical magic values. Doco meaning? Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:41: // // NOTE: the order is not significant. On 2013/05/08 20:26:44, Ami Fischman wrote: > Then why not make ReleaseOwnership implicitly call DestroyDemuxer? Obsolete after redesign https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:54: const MediaStreamDependencyFactory* media_stream_dependency_factory, On 2013/05/08 20:26:44, Ami Fischman wrote: > I'm still pretty unhappy about this business with the singleton that magically > knows who it wants to talk to based on an address (therefore brittle to delete > followed by new reusing the same address). Singletons are bad enough, but this > pattern unnecessarily bleeds that singletonness into multiple classes' public > APIs. > > > ISTM this ought to work by having RVI::createMediaPlayer (or WMPA's ctor) > acquire the singleton factory (or NULL if unavailable) and then pass it through > to the downstream deps that need it. It'll probably mean widening some of the > APIs (like RenderThreadImpl::GetMediaStreamDependencyFactory()) with an > additional parameter, but that seems much more reasonable/readable/robust to me. > Am I missing something? Redesigned following your advice. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.h (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.h:23: class CONTENT_EXPORT RTCVideoDecoderFactoryTv On 2013/05/08 20:26:44, Ami Fischman wrote: > AFAICT this is used only in one place: the mediastreamdependencyfactory cc file. > Why not move the entire class into that .cc file? After refactoring, this class is used elsewhere. https://codereview.chromium.org/14247018/diff/118001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/render_... content/renderer/render_view_impl.cc:2696: #if defined(ENABLE_WEBRTC) && !defined(GOOGLE_TV) On 2013/05/08 20:26:44, Ami Fischman wrote: > The conditional compile & runtime behavior in this function is pretty gnarly... Well, I can't see a good way to improve this. https://codereview.chromium.org/14247018/diff/118001/content/renderer/render_... content/renderer/render_view_impl.cc:2756: #if defined(GOOGLE_TV) On 2013/05/08 20:26:44, Ami Fischman wrote: > shouldn't this be checking for WEBRTC? Done. https://codereview.chromium.org/14247018/diff/118001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/14247018/diff/118001/media/base/android/media... media/base/android/media_player_bridge.cc:49: LOG_IF(WARNING, media_source != MEDIA_SOURCE_URL) On 2013/05/08 20:26:44, Ami Fischman wrote: > I think this is a programming error, in which case this should be > DCHECK_EQ(media_source, MEDIA_SOURCE_URL) << "Unsupported config!"; Done. https://codereview.chromium.org/14247018/diff/118001/media/base/android/media... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/14247018/diff/118001/media/base/android/media... media/base/android/media_player_bridge.h:47: enum MediaSource { On 2013/05/08 20:26:44, Ami Fischman wrote: > MediaSourceType? > (otherwise, e.g. > void OnInitialize(..., media::MediaPlayerBridge::MediaSource media_source, > ...) > looks like it's MSE or GTV MediaSource API, not an enum that specifies the > source of the media data) Done. https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:78: NotifyDemuxerCB notify_demuxer_cb); On 2013/05/08 20:26:44, Ami Fischman wrote: > CB's are usually passed by const-ref Done. https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... File webkit/media/android/media_source_delegate.h (right): https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/med... webkit/media/android/media_source_delegate.h:130: bool is_media_stream_; On 2013/05/08 20:26:44, Ami Fischman wrote: > Should this not be a MediaSourceType? > > In fact since the only place it's used is in determining the access_unit_size, > ISTM it would be better to simply store access_unit_size_ as a member, > initialized appropriately in the InitializeMedia{Source,Stream}() methods. Done. https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/web... File webkit/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:108: // The |audio_renderer_| can be shared by multiple remote streams, and On 2013/05/08 20:26:44, Ami Fischman wrote: > Does this not also apply to multiple local streams? applied s/multipe remote streams/multiple streams/ https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:147: base::Unretained(this)))); On 2013/05/08 20:26:44, Ami Fischman wrote: > Comment about Unretained() from above applies here as well. > In fact, this and the statement at l.135 are identical, so IWBN to extract & > dedup them. Done. https://codereview.chromium.org/14247018/diff/118001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:188: if (audio_renderer_ && paused()) On 2013/05/08 20:26:44, Ami Fischman wrote: > Can these changes to the audio_renderer_'s play/pause/stop behavior be pulled > out into a (much smaller) CL? Done. https://codereview.chromium.org/14247018/diff/124001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/124001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:55: const scoped_refptr<base::MessageLoopProxy>& message_loop) = 0; On 2013/05/08 20:26:44, Ami Fischman wrote: > What is |message_loop|? is it for callback dispatch, or what? > I believe all callsites just use MessageLoopProxy::current(), in which case it's > silly to pass this explicitly through so many layers, and instead should be > replaced by doco that explains the guarantee that callbacks get fired on the > calling thread (for whichever class/method actually cares). N/A after refactoring https://codereview.chromium.org/14247018/diff/124001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/124001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:34: // The size of the access unit to transfer in an IPC. On 2013/05/08 20:26:44, Ami Fischman wrote: > This is silently ignored for mediastreams. The name should reflect the only > case it's applied to. Done.
On 2013/05/13 14:03:47, wonsik wrote: > Gone through a refactoring. Aaron, could you please take a look? There's a > caveat: the test is lagging behind. and I plan to update it tomorrow.
https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... content/renderer/media/rtc_video_decoder_bridge_tv.cc:88: // |inputImage| may be destroyed after this call, so we make a copy of the s/inputImage/inputImage_/ https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... content/renderer/media/rtc_video_decoder_bridge_tv.cc:101: renderTimeMs), As discussed, webrtc expects inputImage._timeStamp rather than renderTimeMs. https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... content/renderer/media/rtc_video_decoder_bridge_tv.cc:135: callback->Decoded(dummy_video_frame); Could you add a doc which says this is needed for stats? https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.cc:123: void RTCDemuxerStream::Destroy() { Could you check if this method is being used? https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.cc:155: base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kOk, front.decoder_buffer); This method can be called from QueueBuffer() which is called from webrtc thread. I am not sure it is safe to call the done callback in the different thread, which is not the one where Read() is called. Maybe chrome media team has an answer for this? https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.cc:217: void RTCVideoDecoderFactoryTv::UpdateSize(const gfx::Size& new_size) { This method looks doing more then update size. How about CreateStream or InitiateStream? https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.cc:221: DCHECK(!init_cb_.is_null()); Is there a good way to see Initialize() will be called before UpdateSize()? https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... File content/renderer/media/rtc_video_decoder_factory_tv.h (right): https://chromiumcodereview.appspot.com/14247018/diff/137001/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.h:26: class CONTENT_EXPORT RTCVideoDecoderFactoryTv Documentation for this class?
dwkang & acolwell: PTAL https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc (right): https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:32: void ReadCallback(media::DemuxerStream::Status status, On 2013/05/08 20:26:44, Ami Fischman wrote: > intentionally ignoring |status|? Added some sanity checks. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:38: void ExpectEqualsAndSignal(int32_t expected_result, int32_t actual_result) { On 2013/05/08 20:26:44, Ami Fischman wrote: > s/_result//g Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:38: void ExpectEqualsAndSignal(int32_t expected_result, int32_t actual_result) { On 2013/05/08 20:26:44, Ami Fischman wrote: > s/_t//g Well they are from the definition of webrtc::VideoDecoder, so I'd like to keep them around. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:107: bridge_->AcquireOwnership(&owner_factory_); On 2013/05/08 20:26:44, Ami Fischman wrote: > What if this fails? > (I'd say CHECK the return value, but that prevents unrelated tests from running > on bots so is frowned upon; instead, ASSERT_TRUE here and ASSERT no errors in > the caller; e.g. > https://code.google.com/p/googletest/wiki/AdvancedGuide?#Asserting_on_Subrout...) Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:120: decoder_thread_event_.Wait(); On 2013/05/08 20:26:44, Ami Fischman wrote: > here and elsewhere, if expectations have already failed, you probably don't want > to keep forging on (see previous comment's link) Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:181: EXPECT_EQ(static_cast<media::Demuxer*>(NULL), On 2013/05/08 20:26:44, Ami Fischman wrote: > FWIW, here and elsewhere, instead of static_cast<LongType::Name*>(NULL) you can > simply test for truth: > EXPECT_FALSE(bridge_->CreateDemuxer(&non_owner_factory_, message_loop_proxy_)); Done. https://codereview.chromium.org/14247018/diff/118001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv_unittest.cc:194: EXPECT_TRUE(bridge_->AcquireOwnership(&owner_factory_)); On 2013/05/08 20:26:44, Ami Fischman wrote: > here and elsewhere, always release. > Since you're using a singleton it would be a good idea to run this suite with > gtest's randomization & repetition support to make sure the tests all clean up > after themselves. > > (e.g. I believe that if this and the first TEST_F above were swapped in order, > the latter would fail). Done --- we're no longer using a singleton, and TearDown would release things. https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:88: // |inputImage| may be destroyed after this call, so we make a copy of the On 2013/05/14 07:57:48, dwkang1 wrote: > s/inputImage/inputImage_/ Done. https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:101: renderTimeMs), On 2013/05/14 07:57:48, dwkang1 wrote: > As discussed, webrtc expects inputImage._timeStamp rather than renderTimeMs. Done. https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:135: callback->Decoded(dummy_video_frame); On 2013/05/14 07:57:48, dwkang1 wrote: > Could you add a doc which says this is needed for stats? Done. https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:123: void RTCDemuxerStream::Destroy() { On 2013/05/14 07:57:48, dwkang1 wrote: > Could you check if this method is being used? Done; it is called at RTCVideoDecoderFactoryTv::ReleaseDemuxer. https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:155: base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kOk, front.decoder_buffer); On 2013/05/14 07:57:48, dwkang1 wrote: > This method can be called from QueueBuffer() which is called from webrtc thread. > I am not sure it is safe to call the done callback in the different thread, > which is not the one where Read() is called. Maybe chrome media team has an > answer for this? read_cb_ is bound to the calling thread's message loop (please see RTCDemuxerStream::Read). https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:217: void RTCVideoDecoderFactoryTv::UpdateSize(const gfx::Size& new_size) { On 2013/05/14 07:57:48, dwkang1 wrote: > This method looks doing more then update size. How about CreateStream or > InitiateStream? Done. https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:221: DCHECK(!init_cb_.is_null()); On 2013/05/14 07:57:48, dwkang1 wrote: > Is there a good way to see Initialize() will be called before UpdateSize()? The order is not deterministic --- made it work both ways. https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.h (right): https://codereview.chromium.org/14247018/diff/137001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.h:26: class CONTENT_EXPORT RTCVideoDecoderFactoryTv On 2013/05/14 07:57:48, dwkang1 wrote: > Documentation for this class? Done.
LGTM
Oops. I missed one thing. except that LGTM. Thanks! https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.cc:209: DemuxerStream* RTCVideoDecoderFactoryTv::GetStream(DemuxerStream::Type type) { Same question for this. Can we say this will be called after InitializeStream()?
https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/m... File content/renderer/media/media_stream_dependency_factory.h (right): https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/m... content/renderer/media/media_stream_dependency_factory.h:214: // outlives this object. Thus weak pointer is sufficient. weak pointer? https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:11: #include "media/base/audio_decoder_config.h" Could you check if these headers are needed until "video_decoder_config.h"? https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:85: base::AutoLock lock(lock_); Do you need this? https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.h (right): https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.h:26: // the |RTCVideoDecoderBridgeTv| objcet (which inherits from s/objcet/object/ https://codereview.chromium.org/14247018/diff/166002/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.h:60: base::Lock lock_; It's unclear which members are protected by this lock.
PTAL https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... File content/renderer/media/media_stream_dependency_factory.h (right): https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... content/renderer/media/media_stream_dependency_factory.h:214: // outlives this object. Thus weak pointer is sufficient. On 2013/05/14 13:28:09, Yuncheol Heo wrote: > weak pointer? raw pointer is considered weak. https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... content/renderer/media/rtc_video_decoder_bridge_tv.h:11: #include "media/base/audio_decoder_config.h" On 2013/05/14 13:28:09, Yuncheol Heo wrote: > Could you check if these headers are needed until "video_decoder_config.h"? Removed unnecessary headers https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.cc:85: base::AutoLock lock(lock_); On 2013/05/14 13:28:09, Yuncheol Heo wrote: > Do you need this? I think so, since video_decoder_config_ may change in RunReadCallback_Locked(). https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.cc:209: DemuxerStream* RTCVideoDecoderFactoryTv::GetStream(DemuxerStream::Type type) { On 2013/05/14 13:24:07, dwkang1 wrote: > Same question for this. Can we say this will be called after InitializeStream()? Yes. GetStream will only be called after init_cb_ is run, which is after InitializeStream is called. https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... File content/renderer/media/rtc_video_decoder_factory_tv.h (right): https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.h:26: // the |RTCVideoDecoderBridgeTv| objcet (which inherits from On 2013/05/14 13:28:09, Yuncheol Heo wrote: > s/objcet/object/ Done. https://chromiumcodereview.appspot.com/14247018/diff/166002/content/renderer/... content/renderer/media/rtc_video_decoder_factory_tv.h:60: base::Lock lock_; On 2013/05/14 13:28:09, Yuncheol Heo wrote: > It's unclear which members are protected by this lock. Done.
Looking pretty good so far. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:34: const webrtc::VideoCodec* codecSettings, nit: change to unix_hacker_style here and below. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:42: if (is_initialized_) nit: collapse this and the 2 conditions above into a single if. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:55: const webrtc::RTPFragmentationHeader* /* fragmentation */, are these commented out to silence a warning? If not then remove the /* & */ here and below. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:136: dummy_video_frame.CreateEmptyFrame(2, 2, 2, 1, 1); doesn't libjingle care about the frame size too? Shouldn't the actual size be passed to this callback as well? https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:28: virtual int32_t InitDecode(const webrtc::VideoCodec* codecSettings, nit: change to unix_hacker_style here and below. s/codecSettings/codec_settings/ https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:79: RTCDemuxerStream::~RTCDemuxerStream() { CHECK(is_destroyed_); } nit: s/CHECK/DCHECK/ I'm assuming you don't actually want to take down the tab in release builds. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:82: LOG(FATAL) << "Does not support audio."; nit: NOTIMPLEMENTED(); https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:94: LOG(FATAL) << "Not reachable."; nit: NOTREACHED(); https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:106: DLOG(INFO) << "frame rate received : " << frame_rate_tracker_.units_second(); nit: s/DLOG(INFO)/DVLOG(1)/ here and below. In general prefer DVLOG over DLOG. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:112: CHECK(read_cb_.is_null()); nit: s/DCHECK/CHECK/ ? https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:123: CHECK(!is_destroyed_); nit: s/CHECK/DCHECK/? https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:135: // No VideoFrame actually reaches cc in Google TV case. We just make nit: What does cc refer to? Update the comment please to make this more clear. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:168: if (type == webrtc::kVideoCodecVP8) { nit: reverse condition and merge with the one above so you can deindent this code. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:179: CHECK(decoder_.get() == decoder); nit: s/CHECK/DCHECK https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:193: CHECK(is_acquired_); nit: s/CHECK/DCHECK/ here and everywhere else below. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:32: decoder_thread_("Test decoeder thread"), nit:s/decoeder/decoder https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:62: memset(&codec_, 0, sizeof(codec_)); I think you can just move all the logic in this method into the constructor. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:72: if (is_demuxer_acquired_) { I believe you can move all of the logic in this method into the destructor. https://codereview.chromium.org/14247018/diff/172001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/render_... content/renderer/render_view_impl.cc:2730: webkit_media::WebMediaPlayerAndroid* web_media_player_android = nit: Use scoped_ptr<webkit_media::WebMediaPlayerAndroid> so you don't have to worry about deleting the object on early returns. https://codereview.chromium.org/14247018/diff/172001/content/renderer/render_... content/renderer/render_view_impl.cc:2744: // |media_stream_impl_| and |factory| outlives web_media_player_android. nit: s/web_media_player_android/|web_media_player_android|/ https://codereview.chromium.org/14247018/diff/172001/media/base/android/media... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/14247018/diff/172001/media/base/android/media... media/base/android/media_player_android.h:40: enum MediaSourceType { nit: s/MediaSourceType/SourceType and s/MEDIA_SOURCE/SOURCE_TYPE ? The 'Media' just seems redundant here. https://codereview.chromium.org/14247018/diff/172001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/14247018/diff/172001/media/base/android/media... media/base/android/media_player_bridge.cc:37: MediaSource media_source, This should be MediaSourceType right? https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:98: OVERRIDE; nit: I think the OVERRIDE is typically on the same line as the ) so I'd suggest breaking the line at the const. https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:101: OVERRIDE; ditto https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/web... File webkit/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:106: destroy_demuxer_cb_.Run(); You should probably clear the media_source_delegate_ before calling this just to make sure it doesn't use a stale demuxer pointer. https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:145: if (media_source) nit: I think this isn't necessary if you move the similar check above outside of the #if. I'm pretty sure media_source & media_stream_client_ shouldn't be set at the same time so you don't really need the else if up there. https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:732: media_stream_client_ = media_stream_client; DCHECK(!demuxer_)?
Thanks! PTAL https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:34: const webrtc::VideoCodec* codecSettings, On 2013/05/14 18:17:47, acolwell wrote: > nit: change to unix_hacker_style here and below. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:42: if (is_initialized_) On 2013/05/14 18:17:47, acolwell wrote: > nit: collapse this and the 2 conditions above into a single if. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:55: const webrtc::RTPFragmentationHeader* /* fragmentation */, On 2013/05/14 18:17:47, acolwell wrote: > are these commented out to silence a warning? If not then remove the /* & */ > here and below. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:136: dummy_video_frame.CreateEmptyFrame(2, 2, 2, 1, 1); On 2013/05/14 18:17:47, acolwell wrote: > doesn't libjingle care about the frame size too? Shouldn't the actual size be > passed to this callback as well? AFAIK libjingle does not report the size of image back to the stats, at least for now. Since creating a frame consumes megabytes of memory and there are a few memcpy's in the pipeline which can be performance burden, I am reluctant to pass the actual size here. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:28: virtual int32_t InitDecode(const webrtc::VideoCodec* codecSettings, On 2013/05/14 18:17:47, acolwell wrote: > nit: change to unix_hacker_style here and below. s/codecSettings/codec_settings/ Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:79: RTCDemuxerStream::~RTCDemuxerStream() { CHECK(is_destroyed_); } On 2013/05/14 18:17:47, acolwell wrote: > nit: s/CHECK/DCHECK/ I'm assuming you don't actually want to take down the tab > in release builds. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:82: LOG(FATAL) << "Does not support audio."; On 2013/05/14 18:17:47, acolwell wrote: > nit: NOTIMPLEMENTED(); Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:94: LOG(FATAL) << "Not reachable."; On 2013/05/14 18:17:47, acolwell wrote: > nit: NOTREACHED(); Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:106: DLOG(INFO) << "frame rate received : " << frame_rate_tracker_.units_second(); On 2013/05/14 18:17:47, acolwell wrote: > nit: s/DLOG(INFO)/DVLOG(1)/ here and below. In general prefer DVLOG over DLOG. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:112: CHECK(read_cb_.is_null()); On 2013/05/14 18:17:47, acolwell wrote: > nit: s/DCHECK/CHECK/ ? Done (assuming you meant the other way around :) ) https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:123: CHECK(!is_destroyed_); On 2013/05/14 18:17:47, acolwell wrote: > nit: s/CHECK/DCHECK/? Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:135: // No VideoFrame actually reaches cc in Google TV case. We just make On 2013/05/14 18:17:47, acolwell wrote: > nit: What does cc refer to? Update the comment please to make this more clear. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:168: if (type == webrtc::kVideoCodecVP8) { On 2013/05/14 18:17:47, acolwell wrote: > nit: reverse condition and merge with the one above so you can deindent this > code. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:179: CHECK(decoder_.get() == decoder); On 2013/05/14 18:17:47, acolwell wrote: > nit: s/CHECK/DCHECK Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:193: CHECK(is_acquired_); On 2013/05/14 18:17:47, acolwell wrote: > nit: s/CHECK/DCHECK/ here and everywhere else below. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:32: decoder_thread_("Test decoeder thread"), On 2013/05/14 18:17:47, acolwell wrote: > nit:s/decoeder/decoder Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:62: memset(&codec_, 0, sizeof(codec_)); On 2013/05/14 18:17:47, acolwell wrote: > I think you can just move all the logic in this method into the constructor. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:72: if (is_demuxer_acquired_) { On 2013/05/14 18:17:47, acolwell wrote: > I believe you can move all of the logic in this method into the destructor. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/render_... content/renderer/render_view_impl.cc:2730: webkit_media::WebMediaPlayerAndroid* web_media_player_android = On 2013/05/14 18:17:47, acolwell wrote: > nit: Use scoped_ptr<webkit_media::WebMediaPlayerAndroid> so you don't have to > worry about deleting the object on early returns. Done. https://codereview.chromium.org/14247018/diff/172001/content/renderer/render_... content/renderer/render_view_impl.cc:2744: // |media_stream_impl_| and |factory| outlives web_media_player_android. On 2013/05/14 18:17:47, acolwell wrote: > nit: s/web_media_player_android/|web_media_player_android|/ Done. https://codereview.chromium.org/14247018/diff/172001/media/base/android/media... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/14247018/diff/172001/media/base/android/media... media/base/android/media_player_android.h:40: enum MediaSourceType { On 2013/05/14 18:17:47, acolwell wrote: > nit: s/MediaSourceType/SourceType and s/MEDIA_SOURCE/SOURCE_TYPE ? The 'Media' > just seems redundant here. Done. https://codereview.chromium.org/14247018/diff/172001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/14247018/diff/172001/media/base/android/media... media/base/android/media_player_bridge.cc:37: MediaSource media_source, On 2013/05/14 18:17:47, acolwell wrote: > This should be MediaSourceType right? Right -- fixed. https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:98: OVERRIDE; On 2013/05/14 18:17:47, acolwell wrote: > nit: I think the OVERRIDE is typically on the same line as the ) so I'd suggest > breaking the line at the const. Done. https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:101: OVERRIDE; On 2013/05/14 18:17:47, acolwell wrote: > ditto Done. https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/web... File webkit/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:106: destroy_demuxer_cb_.Run(); On 2013/05/14 18:17:47, acolwell wrote: > You should probably clear the media_source_delegate_ before calling this just to > make sure it doesn't use a stale demuxer pointer. Done. https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:145: if (media_source) On 2013/05/14 18:17:47, acolwell wrote: > nit: I think this isn't necessary if you move the similar check above outside of > the #if. I'm pretty sure media_source & media_stream_client_ shouldn't be set at > the same time so you don't really need the else if up there. Done --- assuming I understood your intention here correctly.. https://codereview.chromium.org/14247018/diff/172001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:732: media_stream_client_ = media_stream_client; On 2013/05/14 18:17:47, acolwell wrote: > DCHECK(!demuxer_)? Done.
https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:136: dummy_video_frame.CreateEmptyFrame(2, 2, 2, 1, 1); How does not passing the size save much space or time?
Looks really good. Just have a few minor nits and a few questions. https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:136: dummy_video_frame.CreateEmptyFrame(2, 2, 2, 1, 1); On 2013/05/20 14:02:24, wonsik wrote: > On 2013/05/14 18:17:47, acolwell wrote: > > doesn't libjingle care about the frame size too? Shouldn't the actual size be > > passed to this callback as well? > > AFAIK libjingle does not report the size of image back to the stats, at least > for now. Since creating a frame consumes megabytes of memory and there are a few > memcpy's in the pipeline which can be performance burden, I am reluctant to pass > the actual size here. Is there a way we can reuse a frame so we only have one copy or defer allocation until the buffer pointers are actually needed? I'm very wary about lying to libjingle here. It seems like a violation of the API contract. https://codereview.chromium.org/14247018/diff/211001/content/browser/android/... File content/browser/android/media_player_manager_impl.cc (right): https://codereview.chromium.org/14247018/diff/211001/content/browser/android/... content/browser/android/media_player_manager_impl.cc:109: media::MediaPlayerAndroid::SourceType media_source_type, nit:s/media_// https://codereview.chromium.org/14247018/diff/211001/content/common/media/med... File content/common/media/media_player_messages.h (right): https://codereview.chromium.org/14247018/diff/211001/content/common/media/med... content/common/media/media_player_messages.h:151: media::MediaPlayerAndroid::SourceType /* media_source_type */, nit: s/media_// https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:89: if (render_time_ms != -1) { What is supposed to happen when render_time_ms == -1? Why is it ok to queue a buffer w/o a proper timestamp set? https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:24: RTCVideoDecoderBridgeTv(RTCVideoDecoderFactoryTv* factory); nit: needs explicit https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:34: const webrtc::CodecSpecificInfo* codec_specific_info = NULL, Are these default parameters needed? Usually Chrome coding style doesn't allow them. https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:116: RunReadCallback_Locked(); Either put a DCHECK(!is_destroyed_) at the top or add code here to run the read_cb_ with kAborted when is_destroyed == true. https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:125: } Should pending_done_cb_ and/or buffer_queue_ be cleared here as well? https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/w... File content/renderer/media/webmediaplayer_proxy_impl_android.h (right): https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/w... content/renderer/media/webmediaplayer_proxy_impl_android.h:41: media::MediaPlayerAndroid::SourceType media_source_type, nit: ditto https://codereview.chromium.org/14247018/diff/211001/media/base/android/media... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/14247018/diff/211001/media/base/android/media... media/base/android/media_player_android.h:53: SourceType media_source_type, nit:s/media_// https://codereview.chromium.org/14247018/diff/211001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/14247018/diff/211001/media/base/android/media... media/base/android/media_player_bridge.cc:37: SourceType media_source_type, nit:s/media_// https://codereview.chromium.org/14247018/diff/211001/webkit/media/android/web... File webkit/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/14247018/diff/211001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:129: LOG_IF(WARNING, media_source) << "It is absurd to have both media source " nit: DCHECK(!media_source) is sufficient. https://codereview.chromium.org/14247018/diff/211001/webkit/media/android/web... File webkit/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/14247018/diff/211001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.h:235: media::MediaPlayerAndroid::SourceType media_source_type); nit:s/media_// here and everywhere else SourceType is used
PTAL https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/172001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:136: dummy_video_frame.CreateEmptyFrame(2, 2, 2, 1, 1); On 2013/05/20 21:23:21, acolwell wrote: > On 2013/05/20 14:02:24, wonsik wrote: > > On 2013/05/14 18:17:47, acolwell wrote: > > > doesn't libjingle care about the frame size too? Shouldn't the actual size > be > > > passed to this callback as well? > > > > AFAIK libjingle does not report the size of image back to the stats, at least > > for now. Since creating a frame consumes megabytes of memory and there are a > few > > memcpy's in the pipeline which can be performance burden, I am reluctant to > pass > > the actual size here. > > Is there a way we can reuse a frame so we only have one copy or defer allocation > until the buffer pointers are actually needed? I'm very wary about lying to > libjingle here. It seems like a violation of the API contract. OK - not lying now :) https://codereview.chromium.org/14247018/diff/211001/content/browser/android/... File content/browser/android/media_player_manager_impl.cc (right): https://codereview.chromium.org/14247018/diff/211001/content/browser/android/... content/browser/android/media_player_manager_impl.cc:109: media::MediaPlayerAndroid::SourceType media_source_type, On 2013/05/20 21:23:22, acolwell wrote: > nit:s/media_// Done. https://codereview.chromium.org/14247018/diff/211001/content/common/media/med... File content/common/media/media_player_messages.h (right): https://codereview.chromium.org/14247018/diff/211001/content/common/media/med... content/common/media/media_player_messages.h:151: media::MediaPlayerAndroid::SourceType /* media_source_type */, On 2013/05/20 21:23:22, acolwell wrote: > nit: s/media_// Done. https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:89: if (render_time_ms != -1) { On 2013/05/20 21:23:22, acolwell wrote: > What is supposed to happen when render_time_ms == -1? Why is it ok to queue a > buffer w/o a proper timestamp set? Timestamp serves as a cue only, so I think it's fine. https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.h (right): https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:24: RTCVideoDecoderBridgeTv(RTCVideoDecoderFactoryTv* factory); On 2013/05/20 21:23:22, acolwell wrote: > nit: needs explicit Done. https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.h:34: const webrtc::CodecSpecificInfo* codec_specific_info = NULL, On 2013/05/20 21:23:22, acolwell wrote: > Are these default parameters needed? Usually Chrome coding style doesn't allow > them. Done. https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:116: RunReadCallback_Locked(); On 2013/05/20 21:23:22, acolwell wrote: > Either put a DCHECK(!is_destroyed_) at the top or add code here to run the > read_cb_ with kAborted when is_destroyed == true. Done. https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:125: } On 2013/05/20 21:23:22, acolwell wrote: > Should pending_done_cb_ and/or buffer_queue_ be cleared here as well? Done. https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/w... File content/renderer/media/webmediaplayer_proxy_impl_android.h (right): https://codereview.chromium.org/14247018/diff/211001/content/renderer/media/w... content/renderer/media/webmediaplayer_proxy_impl_android.h:41: media::MediaPlayerAndroid::SourceType media_source_type, On 2013/05/20 21:23:22, acolwell wrote: > nit: ditto Done. https://codereview.chromium.org/14247018/diff/211001/media/base/android/media... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/14247018/diff/211001/media/base/android/media... media/base/android/media_player_android.h:53: SourceType media_source_type, On 2013/05/20 21:23:22, acolwell wrote: > nit:s/media_// Done. https://codereview.chromium.org/14247018/diff/211001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/14247018/diff/211001/media/base/android/media... media/base/android/media_player_bridge.cc:37: SourceType media_source_type, On 2013/05/20 21:23:22, acolwell wrote: > nit:s/media_// Done. https://codereview.chromium.org/14247018/diff/211001/webkit/media/android/web... File webkit/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/14247018/diff/211001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.cc:129: LOG_IF(WARNING, media_source) << "It is absurd to have both media source " On 2013/05/20 21:23:22, acolwell wrote: > nit: DCHECK(!media_source) is sufficient. Done. https://codereview.chromium.org/14247018/diff/211001/webkit/media/android/web... File webkit/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/14247018/diff/211001/webkit/media/android/web... webkit/media/android/webmediaplayer_android.h:235: media::MediaPlayerAndroid::SourceType media_source_type); On 2013/05/20 21:23:22, acolwell wrote: > nit:s/media_// here and everywhere else SourceType is used Done.
lgtm https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:133: // 2x2 y values and 1x1 u & v values each. nit: remove comment since it doesn't apply anymore. https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:112: read_cb.Run(DemuxerStream::kAborted, NULL); s/read_cb/media::BindToLoop(base::MessageLoopProxy::current(), read_cb)/ so that the callback is always run from a new callstack. https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:211: init_cb_ = cb; nit: I believe you should use media::BindToLoop(base::MessageLoopProxy::current(), cb) here as well. https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc (right): https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:106: const webrtc::CodecSpecificInfo* info = NULL, nit: remove default values here since they appear to be unnecessary. https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:157: const webrtc::CodecSpecificInfo* info = NULL, nit: Remove default values since they aren't used anywhere https://codereview.chromium.org/14247018/diff/226001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/226001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:415: demuxer_->InitializeDemuxer(demuxer); nit:s/Initialize/SetWrapped/ or s/Initialize/SetChild/ or something similar. It looks weird to have an InitializeDemuxer() call followed by a demuxer_->Initialize() call.
jam: content/renderer/render_view_impl.cc and *.gypi https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right): https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_bridge_tv.cc:133: // 2x2 y values and 1x1 u & v values each. On 2013/05/21 15:51:47, acolwell wrote: > nit: remove comment since it doesn't apply anymore. Done. https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv.cc (right): https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:112: read_cb.Run(DemuxerStream::kAborted, NULL); On 2013/05/21 15:51:47, acolwell wrote: > s/read_cb/media::BindToLoop(base::MessageLoopProxy::current(), read_cb)/ so that > the callback is always run from a new callstack. Done. https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv.cc:211: init_cb_ = cb; On 2013/05/21 15:51:47, acolwell wrote: > nit: I believe you should use > media::BindToLoop(base::MessageLoopProxy::current(), cb) here as well. Done. https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc (right): https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:106: const webrtc::CodecSpecificInfo* info = NULL, On 2013/05/21 15:51:47, acolwell wrote: > nit: remove default values here since they appear to be unnecessary. Done. https://codereview.chromium.org/14247018/diff/226001/content/renderer/media/r... content/renderer/media/rtc_video_decoder_factory_tv_unittest.cc:157: const webrtc::CodecSpecificInfo* info = NULL, On 2013/05/21 15:51:47, acolwell wrote: > nit: Remove default values since they aren't used anywhere Done. https://codereview.chromium.org/14247018/diff/226001/webkit/media/android/med... File webkit/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/14247018/diff/226001/webkit/media/android/med... webkit/media/android/media_source_delegate.cc:415: demuxer_->InitializeDemuxer(demuxer); On 2013/05/21 15:51:47, acolwell wrote: > nit:s/Initialize/SetWrapped/ or s/Initialize/SetChild/ or something similar. It > looks weird to have an InitializeDemuxer() call followed by a > demuxer_->Initialize() call. Done.
A friendly reminder :) jam: content/renderer/render_view_impl.cc and *.gypi
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wonsik@chromium.org/14247018/258001
Failed to apply patch for
content/renderer/media/media_stream_dependency_factory.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/renderer/media/media_stream_dependency_factory.cc
Hunk #1 succeeded at 39 (offset -1 lines).
Hunk #2 succeeded at 216 (offset -1 lines).
Hunk #3 FAILED at 486.
1 out of 3 hunks FAILED -- saving rejects to file
content/renderer/media/media_stream_dependency_factory.cc.rej
Patch: content/renderer/media/media_stream_dependency_factory.cc
Index: content/renderer/media/media_stream_dependency_factory.cc
diff --git a/content/renderer/media/media_stream_dependency_factory.cc
b/content/renderer/media/media_stream_dependency_factory.cc
index
4faebcf951cb89a465e2a6959fc80277a20c8822..7a230847b5c7288531a7d1ba3a198ee793fa57b8
100644
--- a/content/renderer/media/media_stream_dependency_factory.cc
+++ b/content/renderer/media/media_stream_dependency_factory.cc
@@ -40,6 +40,10 @@
#include "net/socket/nss_ssl_util.h"
#endif
+#if defined(GOOGLE_TV)
+#include "content/renderer/media/rtc_video_decoder_factory_tv.h"
+#endif
+
namespace content {
// The constraint key for the PeerConnection constructor for enabling
diagnostic
@@ -213,6 +217,9 @@ MediaStreamDependencyFactory::MediaStreamDependencyFactory(
VideoCaptureImplManager* vc_manager,
P2PSocketDispatcher* p2p_socket_dispatcher)
: network_manager_(NULL),
+#if defined(GOOGLE_TV)
+ decoder_factory_tv_(NULL),
+#endif
vc_manager_(vc_manager),
p2p_socket_dispatcher_(p2p_socket_dispatcher),
signaling_thread_(NULL),
@@ -479,9 +486,20 @@ bool
MediaStreamDependencyFactory::CreatePeerConnectionFactory() {
if (!pc_factory_) {
DCHECK(!audio_device_);
audio_device_ = new WebRtcAudioDeviceImpl();
+
+ cricket::WebRtcVideoDecoderFactory* decoder_factory = NULL;
+#if defined(GOOGLE_TV)
+ // PeerConnectionFactory will hold the ownership of this
+ // VideoDecoderFactory.
+ decoder_factory = decoder_factory_tv_ = new RTCVideoDecoderFactoryTv;
+#endif
+
scoped_refptr<webrtc::PeerConnectionFactoryInterface> factory(
- webrtc::CreatePeerConnectionFactory(
- worker_thread_, signaling_thread_, audio_device_, NULL, NULL));
+ webrtc::CreatePeerConnectionFactory(worker_thread_,
+ signaling_thread_,
+ audio_device_,
+ NULL,
+ decoder_factory));
if (factory)
pc_factory_ = factory;
else
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wonsik@chromium.org/14247018/262002
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wonsik@chromium.org/14247018/266004
Message was sent while issue was closed.
Change committed as 203706 |
