Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10073016:
Upstream WebMediaPlayerAndroid as WebKit::WebMediaPlayer implementation on android. (Closed)
|
Created:
8 years, 8 months ago by qinmin Modified:
8 years, 8 months ago Reviewers:
scherkus (not reviewing) CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpstream WebMediaPlayerAndroid as WebKit::WebMediaPlayer implementation on android.
I've seperated the WebMediaPlayerAndroid into several changes so that each change will be easier to understand.
This is the first change in the series. It implements the basic functionalities needed for WebMediaPlayer on android.
Here are the things that are missing and will be addressed in future changes:
1. Only audio is working after this change (for video, you can only hear audio now). Video will be in the next change.
2. Fullscreen implementation is missing in this change, it will come after the video change.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133971
Patch Set 1 #
Total comments: 95
Patch Set 2 : addressing feedbacks #
Total comments: 20
Patch Set 3 : addressing feedbacks #
Total comments: 8
Patch Set 4 : small changes to address comments #
Messages
Total messages: 10 (0 generated)
This is the the first change for WebMediaPlayer implementation on android.
http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... File webkit/media/android/webmediaplayer_android.cc (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:43: base::TimeDelta ConvertSecondsToTimestamp(float seconds) { this code is duplicated http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:68: : frame_(frame), WMPA doesn't use this http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:70: render_message_loop_(base::MessageLoopProxy::current()), WMPA doesn't use this http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:71: media_player_(NULL), for scoped_ptr/scoped_refptr we typically leave these out http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:72: natural_size_(WebSize(0,0)), this is handles by default ctor -- no need to have http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:93: media_player_.reset(NULL); do you need this line? the dtor should call this automatically http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:96: video_frame_.reset(NULL); ditto http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:100: incognito_mode_ = incognito_mode; when is this called? is it guaranteed this will get called before we start loading? also, can we query for incognito mode? why does it need to be a class static? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:104: url_ = url; most of your url_ usage involves immediately converting it to a GURL -- why not store a GURL instead? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:111: // play/seek/fullsceen button is clicked. as a web page author can't I work around this by having javascript code that calls play() or sets currentTime, in which case what's the point? this code would be vastly simplified if we didn't have to do such a thing... http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:120: if (skip_loading_) { you can replace skip_loading_ with a check for media_player_.get() or even rewrite this code to handle the file:// case first and return early http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:121: // FIXME(qinmin): we need a method to calculate the duration of the media. s/FIXME/TODO http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:126: duration_ = 100; you shouldn't need a duration_ parameter at all in fact all of this is way too hacky if you want to disable seeking temporarily you should be able to implement the following for maxTimeSeekable(): if (!prepared_) return 0; return duration(); http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:158: // We don't need to load media if pause is called(). pause() instead of called()? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:165: time_to_seek_ = seconds; perhaps a better variable name is "pending_seek_" to line up with the other pending_xxx_ variables http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:170: LoadUrl(url_); curly braces please http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:182: // Deprecated. for these and others: if they're deprecated are there bugs filed? should we remove these? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:213: if(!gurl.has_path()) space between if and ( http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:218: return mime.find("audio") == std::string::npos; should this be "audio/"? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:221: return natural_size_.width != 0 && natural_size_.height != 0; how about !natural_size_.isEmpty() instead? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:251: // current_time_ to media duration when PLAYBACK_COMPLETE message is by "PLAYBACK_COMPLETE message" do you mean OnPlaybackComplete()? I'd update docs http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:254: return std::max(current_time_, current_time_ is completely misleading as it's either 0 or duration() why not use a boolean to track when OnPlaybackComplete() has been called and instead of confusing std::max() + documentation we have a much simpler: if (playback_completed_) return duration(); http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:255: (float) media_player_->GetCurrentTime().InSecondsF()); static_cast<> http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:337: new_buffered[0].start = 0.0f; WebTimeRanges's default ctor initializes to 0 http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:339: time_ranges_.swap(new_buffered); why the swap? how often is OnMediaPrepared() called? shouldn't it be once-per-WMPA-lifetime? if anything you could do: time_ranges_.clear(); time_ranges_.push_back(WebTimeRanges()); ...or something similar http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:362: // We have to update webkit about the new duration. even in the file:// case isn't the duration also unknown for a temporary amount of time? so shouldn't we *always* play it safe and rescale time_to_seek_? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:388: if (time_ranges_.size() > 0) { would OnBufferingUpdate() ever get called before OnMediaPrepared()? if so that seems very strange -- maybe this should be a DCHECK() instead? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:393: buffered_bytes_++; FYI this is a pretty horrible trick! can you at least add a TODO? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:408: // When playing an bogus URL or bad file, we fire an MEDIA_ERROR_UNKNOWN. s/an/a s/,// http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:410: // file, we default an MEDIA_ERROR_UNKNOWN to FormatError. s/an/a s/,// http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:448: void WebMediaPlayerAndroid::SetVideoSurface( nit: this can fit on single line http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:455: if (!media_player_.get()) { this check isn't needed -- you only call this method once from inside LoadUrl() -- hence suggesting you combine the methods if anything, it'd make more sense as a CHECK http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:463: void WebMediaPlayerAndroid::LoadUrl(WebURL url) { AFAIK this only gets called once during the lifetime of the class (webkit recreates the WMPA object if the URL changes) you should have a CHECK(!media_player_.get()) to enforce it http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:467: media_player_->Reset(); not needed since LoadUrl() should only get called to create a media_player_ http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:470: if (cookie_jar_ != NULL) is it valid to pass in a NULL cookie_jar to this class? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:480: nit: remove blank line http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:498: if (seconds == 0.0f && currentTime() == 0.0f) { what's this for? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:509: // flag is set. you've already covered this in the .h -- I'd remove the // ------------ comments http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... File webkit/media/android/webmediaplayer_android.h (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:102: // composite thread if useThreadedCompositor flag is set. s/composite/compositor http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:102: // composite thread if useThreadedCompositor flag is set. what is useThreadedCompositor? I don't see this flag anywhere http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:107: void OnMediaPrepared(); do these need OVERRIDE or are they callbacks? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:119: // In case |media_player_| is deleted, we call this function to get this comment is inaccurate -- the real reason why media_player_ is NULL is because you delayed loading there is no code inside WMPA that deletes the media_player_ object after it has been created http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:124: void LoadUrl(WebKit::WebURL url); why bother having the arg? you always pass url_, which this method can access itself also I think LoadUrl() should get collapsed into InitializeMediaPlayer() -- there's never a situation where you will recreate a media player with a different URL http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... File webkit/media/android/webmediaplayer_proxy_android.h (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_proxy_android.h:21: : public base::RefCountedThreadSafe<WebMediaPlayerProxyAndroid> { AFAIK this class can be vastly simplified if WMPA inherited from SupportsWeakPtr<> since you're essentially re-writing code that can be automagically generated for you! you would then pass in a WeakPtr version of WMPA to this class, which would then do the following: void WebMediaPlayerProxyAndroid::PlaybackCompleteCallback() { render_loop_->PostTask(FROM_HERE, base::Bind( &WebMediaPlayerProxyAndroid::PlaybackCompleteTask, weak_ptr_version_of_wmpa_)); } that way instead of ~WMPA() you don't need to call SetWebMediaPlayer(NULL) and you can delete half of this class that also means you can likely pass in the instance of WMPA inside the constructor of this class http://codereview.chromium.org/10073016/diff/1/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/webkit_media.gypi... webkit/media/webkit_media.gypi:17: 'android/audio_decoder_android.cc', not seeing this file
For android, the mediaplayer could get released under many circumstances (switching tabs, when found idle, when app goes to background, etc). So the android mediaplayer could be created/deleted during the life cycle of webmediaplayer. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... File webkit/media/android/webmediaplayer_android.cc (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:43: base::TimeDelta ConvertSecondsToTimestamp(float seconds) { move the function to webmediaplayer_util.h, and also changed webmediaplayer_impl.cc On 2012/04/17 03:57:31, scherkus wrote: > this code is duplicated http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:68: : frame_(frame), This will be used by the fullscreen view code to get the webview. Removed for now, I will add it back later when upstreaming the fullscreen view code On 2012/04/17 03:57:31, scherkus wrote: > WMPA doesn't use this http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:70: render_message_loop_(base::MessageLoopProxy::current()), On 2012/04/17 03:57:31, scherkus wrote: > WMPA doesn't use this Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:71: media_player_(NULL), On 2012/04/17 03:57:31, scherkus wrote: > for scoped_ptr/scoped_refptr we typically leave these out Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:72: natural_size_(WebSize(0,0)), On 2012/04/17 03:57:31, scherkus wrote: > this is handles by default ctor -- no need to have Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:93: media_player_.reset(NULL); On 2012/04/17 03:57:31, scherkus wrote: > do you need this line? the dtor should call this automatically Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:96: video_frame_.reset(NULL); On 2012/04/17 03:57:31, scherkus wrote: > ditto Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:100: incognito_mode_ = incognito_mode; This will be called by chrome/browser/chrome_content_browser_client.cc. This method is called when render process is created, so it should always get called before we start loading. Incognito is browser side setting, so renderer side should not know about this. And also webkit/media should not have dependencies on chrome dir. And the incoginto setting applies to all WMPAs created in that process, it makes sense to make it static. On 2012/04/17 03:57:31, scherkus wrote: > when is this called? is it guaranteed this will get called before we start > loading? > > also, can we query for incognito mode? why does it need to be a class static? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:104: url_ = url; On 2012/04/17 03:57:31, scherkus wrote: > most of your url_ usage involves immediately converting it to a GURL -- why not > store a GURL instead? Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:111: // play/seek/fullsceen button is clicked. No, calling play() will not work as there are gesture requirement in HTMLMediaElemet. The major concern here is both performance and data. And also to avoid bugs in video decoders. For example, for youtube webm videos, calling mediaplayer->prepare will trigger a download of the whole file before start parsing the metadata on ICS. This greatly slows down page scrolling and adds tens of megabytes of data usage. For files, we want the same behavior as desktop so that DRT tests can pass. Ultimately, we should have a separate thread on render process that calls android MediaMetadataRetriever::extractMetadata() to retrieve the metadata here. But that is still in the planning. On 2012/04/17 03:57:31, scherkus wrote: > as a web page author can't I work around this by having javascript code that > calls play() or sets currentTime, in which case what's the point? > > this code would be vastly simplified if we didn't have to do such a thing... http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:120: if (skip_loading_) { rewrite the handle to file:// case On 2012/04/17 03:57:31, scherkus wrote: > you can replace skip_loading_ with a check for media_player_.get() or even > rewrite this code to handle the file:// case first and return early http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:121: // FIXME(qinmin): we need a method to calculate the duration of the media. On 2012/04/17 03:57:31, scherkus wrote: > s/FIXME/TODO Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:126: duration_ = 100; We need the duration_ for many reasons: 1. MediaPlayer will be released when switching tabs, so when switching back, if the user want to perform a seek, having a duration_ at hand helps as we don't need to rescale the seek position. 2. If user hit the fullscreen button, we need some duration value (in the case mediaplayer is not prepared) to send to the fullscreen view so that the media controller will show up correctly. 3. we want to let users able to seek. Presetting the duration_ to 100 is not very desirable. We should be able to get around it when we have an additional thread on the renderer process to call MediaMetadataRetriever::extractMetadata(). An alternative here is that I remove the preset value here, so duration_ will be initialized to 0. In that case, the first time user sees the media, he cannot seek it but he can play it(my guess, I haven't tested this though). After he starts playing it we should be able to get the duration_ . And he can perform seek on the media the next time he visit the page. On 2012/04/17 03:57:31, scherkus wrote: > you shouldn't need a duration_ parameter at all > > in fact all of this is way too hacky > > if you want to disable seeking temporarily you should be able to implement the > following for maxTimeSeekable(): > > if (!prepared_) > return 0; > return duration(); http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:158: // We don't need to load media if pause is called(). On 2012/04/17 03:57:31, scherkus wrote: > pause() instead of called()? Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:165: time_to_seek_ = seconds; On 2012/04/17 03:57:31, scherkus wrote: > perhaps a better variable name is "pending_seek_" to line up with the other > pending_xxx_ variables Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:170: LoadUrl(url_); On 2012/04/17 03:57:31, scherkus wrote: > curly braces please Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:182: // Deprecated. No, there are no bugs filed. Added a todo for this. These functions are no longer being called. On 2012/04/17 03:57:31, scherkus wrote: > for these and others: if they're deprecated are there bugs filed? should we > remove these? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:213: if(!gurl.has_path()) On 2012/04/17 03:57:31, scherkus wrote: > space between if and ( Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:218: return mime.find("audio") == std::string::npos; Done, but should have the same effect for all current media mime type. On 2012/04/17 03:57:31, scherkus wrote: > should this be "audio/"? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:221: return natural_size_.width != 0 && natural_size_.height != 0; On 2012/04/17 03:57:31, scherkus wrote: > how about !natural_size_.isEmpty() instead? Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:251: // current_time_ to media duration when PLAYBACK_COMPLETE message is On 2012/04/17 03:57:31, scherkus wrote: > by "PLAYBACK_COMPLETE message" do you mean OnPlaybackComplete()? I'd update docs Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:254: return std::max(current_time_, current_time was used to send the current time information to the fullscreen view, and we have a timer object that regularly pulls the current time from the media player. Removed for now as I can add it back when upstreaming the fullscreen video. One issue related to playback_completed_ is when we need to reset it to false. I am putting that into seek() call, hope this does not cause any problems. On 2012/04/17 03:57:31, scherkus wrote: > current_time_ is completely misleading as it's either 0 or duration() > > why not use a boolean to track when OnPlaybackComplete() has been called and > instead of confusing std::max() + documentation we have a much simpler: > > if (playback_completed_) > return duration(); http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:255: (float) media_player_->GetCurrentTime().InSecondsF()); On 2012/04/17 03:57:31, scherkus wrote: > static_cast<> Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:337: new_buffered[0].start = 0.0f; On 2012/04/17 03:57:31, scherkus wrote: > WebTimeRanges's default ctor initializes to 0 Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:339: time_ranges_.swap(new_buffered); This could be called many times as MediaPlayer could got destroyed in many situations (switching tabs, moving app to background, player become idle...) WebVector does not have clear() and push_back() function, only swap() On 2012/04/17 03:57:31, scherkus wrote: > why the swap? how often is OnMediaPrepared() called? shouldn't it be > once-per-WMPA-lifetime? > > if anything you could do: > time_ranges_.clear(); > time_ranges_.push_back(WebTimeRanges()); > > ...or something similar http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:362: // We have to update webkit about the new duration. So here is what happens: WebKit will call duration() when readyState is set to HaveEnoughData. So for url case, after we set the ready state to HaveEnoughData in the above code, webkit will immediately get a duration of 100 seconds. This allows the users to do seek/play immediately. However, for the file:// case, because the ready state is not set here, so user cannot perform any operation on the media controller (the controller is disabled). Only until onPrepared() is called, webkit will get the duration and the user will be able to seek/play. And since we will have the duration when onPrepared() is called, so webkit will know the exact duration of the media. On 2012/04/17 03:57:31, scherkus wrote: > even in the file:// case isn't the duration also unknown for a temporary amount > of time? > > so shouldn't we *always* play it safe and rescale time_to_seek_? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:388: if (time_ranges_.size() > 0) { Yes, that could happen as mediaplayer could get destroyed and there is a previous time_range_. On 2012/04/17 03:57:31, scherkus wrote: > would OnBufferingUpdate() ever get called before OnMediaPrepared()? > > if so that seems very strange -- maybe this should be a DCHECK() instead? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:393: buffered_bytes_++; TODO added, android mediaplayer does not provide us with these size related callbacks, so no easy fix for now. On 2012/04/17 03:57:31, scherkus wrote: > FYI this is a pretty horrible trick! > > can you at least add a TODO? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:408: // When playing an bogus URL or bad file, we fire an MEDIA_ERROR_UNKNOWN. On 2012/04/17 03:57:31, scherkus wrote: > s/an/a > > s/,// Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:410: // file, we default an MEDIA_ERROR_UNKNOWN to FormatError. On 2012/04/17 03:57:31, scherkus wrote: > s/an/a > > s/,// Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:448: void WebMediaPlayerAndroid::SetVideoSurface( On 2012/04/17 03:57:31, scherkus wrote: > nit: this can fit on single line Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:455: if (!media_player_.get()) { Changed it to a CHECK instead. On 2012/04/17 03:57:31, scherkus wrote: > this check isn't needed -- you only call this method once from inside LoadUrl() > -- hence suggesting you combine the methods > > if anything, it'd make more sense as a CHECK http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:463: void WebMediaPlayerAndroid::LoadUrl(WebURL url) { This could get called many times since we will release the mediaplayer when it is inactive or when switching tabs. On 2012/04/17 03:57:31, scherkus wrote: > AFAIK this only gets called once during the lifetime of the class (webkit > recreates the WMPA object if the URL changes) > > you should have a CHECK(!media_player_.get()) to enforce it http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:467: media_player_->Reset(); On 2012/04/17 03:57:31, scherkus wrote: > not needed since LoadUrl() should only get called to create a media_player_ Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:470: if (cookie_jar_ != NULL) Yes, for DRT case. On 2012/04/17 03:57:31, scherkus wrote: > is it valid to pass in a NULL cookie_jar to this class? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:480: On 2012/04/17 03:57:31, scherkus wrote: > nit: remove blank line Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:498: if (seconds == 0.0f && currentTime() == 0.0f) { This is just to save some unnecessary calls to the android media player. Removed as it only helps the case when the initial seek(0) is called. On 2012/04/17 03:57:31, scherkus wrote: > what's this for? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:509: // flag is set. On 2012/04/17 03:57:31, scherkus wrote: > you've already covered this in the .h -- I'd remove the // ------------ comments Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... File webkit/media/android/webmediaplayer_android.h (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:102: // composite thread if useThreadedCompositor flag is set. On 2012/04/17 03:57:31, scherkus wrote: > s/composite/compositor Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:102: // composite thread if useThreadedCompositor flag is set. This flag is outdated. When compositor thread was first developed for android, we introduced that flag at the beginning so the development of compositor thread will not interfere with other developers. On 2012/04/17 03:57:31, scherkus wrote: > what is useThreadedCompositor? I don't see this flag anywhere http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:107: void OnMediaPrepared(); No, OVERRIDE is only for inherited methods. These callbacks are not inherited. On 2012/04/17 03:57:31, scherkus wrote: > do these need OVERRIDE or are they callbacks? http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:119: // In case |media_player_| is deleted, we call this function to get For this change, you are right. But for the upcoming changes, i will add methods to release media player when we found a media player is idle. This is due to the limited resources on the mobile devices. For example, xoom can support up to 5 video decoders at the same time. As a result, opening 5 youtube pages will make us running out of resources. Updated the comment to reflect this. On 2012/04/17 03:57:31, scherkus wrote: > this comment is inaccurate -- the real reason why media_player_ is NULL is > because you delayed loading > > there is no code inside WMPA that deletes the media_player_ object after it has > been created http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.h:124: void LoadUrl(WebKit::WebURL url); On 2012/04/17 03:57:31, scherkus wrote: > why bother having the arg? you always pass url_, which this method can access > itself > > also I think LoadUrl() should get collapsed into InitializeMediaPlayer() -- > there's never a situation where you will recreate a media player with a > different URL Done. http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... File webkit/media/android/webmediaplayer_proxy_android.h (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_proxy_android.h:21: : public base::RefCountedThreadSafe<WebMediaPlayerProxyAndroid> { Done. SetWebMediaPlayer(NULL) actually can be called outside ~WMPA(). That happens when we release the |mediaplayer| and do not want to receive any future callbacks that are currently posted. I agree using WeakPtr is a better choice. And I am now putting the logic in SetWebMediaPlayer(NULL) into WMPA. On 2012/04/17 03:57:31, scherkus wrote: > AFAIK this class can be vastly simplified if WMPA inherited from > SupportsWeakPtr<> since you're essentially re-writing code that can be > automagically generated for you! > > you would then pass in a WeakPtr version of WMPA to this class, which would then > do the following: > void WebMediaPlayerProxyAndroid::PlaybackCompleteCallback() { > render_loop_->PostTask(FROM_HERE, base::Bind( > &WebMediaPlayerProxyAndroid::PlaybackCompleteTask, > weak_ptr_version_of_wmpa_)); > } > > that way instead of ~WMPA() you don't need to call SetWebMediaPlayer(NULL) and > you can delete half of this class > > that also means you can likely pass in the instance of WMPA inside the > constructor of this class http://codereview.chromium.org/10073016/diff/1/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/webkit_media.gypi... webkit/media/webkit_media.gypi:17: 'android/audio_decoder_android.cc', This should be a diff error, the file and this line should have already been there On 2012/04/17 03:57:31, scherkus wrote: > not seeing this file
http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... File webkit/media/android/webmediaplayer_android.cc (right): http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:100: incognito_mode_ = incognito_mode; On 2012/04/18 20:06:30, qinmin wrote: > This will be called by chrome/browser/chrome_content_browser_client.cc. This > method is called when render process is created, so it should always get called > before we start loading. > > Incognito is browser side setting, so renderer side should not know about this. > And also webkit/media should not have dependencies on chrome dir. And the > incoginto setting applies to all WMPAs created in that process, it makes sense > to make it static. What you describe are all implementation details of Chrome and not WebKit. It's possible that some other consumer of Chromium WebKit may build a browser that will have some media players hide their URL history while other media players do not. The concept of separate processes isn't a concept that belongs in WebKit code. I also noticed the MediaPlayerBridge code refers to this parameter as hide_url_log and not incognito. Is it possible to move this argument to the constructor + rename it? > On 2012/04/17 03:57:31, scherkus wrote: > > when is this called? is it guaranteed this will get called before we start > > loading? > > > > also, can we query for incognito mode? why does it need to be a class static? > http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:111: // play/seek/fullsceen button is clicked. Thanks for the explanation! On 2012/04/18 20:06:30, qinmin wrote: > No, calling play() will not work as there are gesture requirement in > HTMLMediaElemet. > The major concern here is both performance and data. And also to avoid bugs in > video decoders. For example, for youtube webm videos, calling > mediaplayer->prepare will trigger a download of the whole file before start > parsing the metadata on ICS. This greatly slows down page scrolling and adds > tens of megabytes of data usage. > For files, we want the same behavior as desktop so that DRT tests can pass. > > Ultimately, we should have a separate thread on render process that calls > android MediaMetadataRetriever::extractMetadata() to retrieve the metadata here. > But that is still in the planning. > > On 2012/04/17 03:57:31, scherkus wrote: > > as a web page author can't I work around this by having javascript code that > > calls play() or sets currentTime, in which case what's the point? > > > > this code would be vastly simplified if we didn't have to do such a thing... > http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:388: if (time_ranges_.size() > 0) { But time_ranges_ is never reset back to size 0 and that call ordering only happens when building consecutive media players. For the _first_ media player created it shouldn't be possible to receive a buffering update, right? Also, see my comments above about side-stepping this issue by initializing time_ranges_ to size 1 in the first place On 2012/04/18 20:06:30, qinmin wrote: > Yes, that could happen as mediaplayer could get destroyed > and there is a previous time_range_. > > On 2012/04/17 03:57:31, scherkus wrote: > > would OnBufferingUpdate() ever get called before OnMediaPrepared()? > > > > if so that seems very strange -- maybe this should be a DCHECK() instead? > http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:393: buffered_bytes_++; You should send a patch to WebKit :) In fact in response to this I chatted w/ some folks and we agree that asking for bytes to check for progress isn't a good API so you can expect this to change at some point. On 2012/04/18 20:06:30, qinmin wrote: > TODO added, android mediaplayer does not provide us with these size related > callbacks, so no easy fix for now. > > On 2012/04/17 03:57:31, scherkus wrote: > > FYI this is a pretty horrible trick! > > > > can you at least add a TODO? > http://codereview.chromium.org/10073016/diff/1/webkit/media/android/webmediap... webkit/media/android/webmediaplayer_android.cc:470: if (cookie_jar_ != NULL) There's no DRT-specific cookie jar implementation we can use? On 2012/04/18 20:06:30, qinmin wrote: > Yes, for DRT case. > > On 2012/04/17 03:57:31, scherkus wrote: > > is it valid to pass in a NULL cookie_jar to this class? > http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... File webkit/media/android/webmediaplayer_android.cc (right): http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:75: // Calling InitializeMediaPlayer will cause android mediaplayer to start nit: append () to the end of function names in comments http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:90: duration_ = 100; can we get a constant for this? also does 100 value line up with the 100.0f scaling factor for pending_seek_? both should be constants -- you can a file statics like: static const float kTemporaryDuration = 100.0f; or something like that http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:130: // Reset playback_completed_ so that we return the correct current time. nit: add || around variable names http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:226: return static_cast<float> (media_player_->GetCurrentTime().InSecondsF()); nit: no space between > and ( http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:247: return time_ranges_; this should really be called buffered_ http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:307: WebKit::WebTimeRanges new_buffered(static_cast<size_t>(1)); is there any harm in always having time_ranges_ set to size 1 in the constructor and simply update the end value in OnBufferingUpdate() w/o checking that this has been set? http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:332: // Scale the pending_seek_ according to the new duration. nit: add || around var names http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... File webkit/media/android/webmediaplayer_android.h (right): http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.h:120: // Create a media player to load the url_ and prepare for playback. nit: "the url_" -> "|url_|" http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... File webkit/media/android/webmediaplayer_proxy_android.cc (right): http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_proxy_android.cc:28: render_loop_->PostTask(FROM_HERE, nit on style: render_loop->PostTask(FROM_HERE, base::Bind( ____&WebMediaPlayerAndroid::OnMediaError, webmediplayer_, ____error_type)); here + below http://codereview.chromium.org/10073016/diff/7001/webkit/media/webmediaplayer... File webkit/media/webmediaplayer_util.h (right): http://codereview.chromium.org/10073016/diff/7001/webkit/media/webmediaplayer... webkit/media/webmediaplayer_util.h:16: base::TimeDelta ConvertSecondsToTimestamp(float seconds) { let's get a .cc for this
http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... File webkit/media/android/webmediaplayer_android.cc (right): http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:75: // Calling InitializeMediaPlayer will cause android mediaplayer to start On 2012/04/21 03:02:05, scherkus wrote: > nit: append () to the end of function names in comments Done. http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:90: duration_ = 100; Done. Added a const and TODO in the beginning of this file. On 2012/04/21 03:02:05, scherkus wrote: > can we get a constant for this? > > also does 100 value line up with the 100.0f scaling factor for pending_seek_? > > both should be constants -- you can a file statics like: > static const float kTemporaryDuration = 100.0f; > > or something like that http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:130: // Reset playback_completed_ so that we return the correct current time. On 2012/04/21 03:02:05, scherkus wrote: > nit: add || around variable names Done. http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:226: return static_cast<float> (media_player_->GetCurrentTime().InSecondsF()); On 2012/04/21 03:02:05, scherkus wrote: > nit: no space between > and ( Done. http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:247: return time_ranges_; On 2012/04/21 03:02:05, scherkus wrote: > this should really be called buffered_ Done. http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:307: WebKit::WebTimeRanges new_buffered(static_cast<size_t>(1)); On 2012/04/21 03:02:05, scherkus wrote: > is there any harm in always having time_ranges_ set to size 1 in the constructor > and simply update the end value in OnBufferingUpdate() w/o checking that this > has been set? Done. http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.cc:332: // Scale the pending_seek_ according to the new duration. On 2012/04/21 03:02:05, scherkus wrote: > nit: add || around var names Done. http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... File webkit/media/android/webmediaplayer_android.h (right): http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_android.h:120: // Create a media player to load the url_ and prepare for playback. On 2012/04/21 03:02:05, scherkus wrote: > nit: "the url_" -> "|url_|" Done. http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... File webkit/media/android/webmediaplayer_proxy_android.cc (right): http://codereview.chromium.org/10073016/diff/7001/webkit/media/android/webmed... webkit/media/android/webmediaplayer_proxy_android.cc:28: render_loop_->PostTask(FROM_HERE, On 2012/04/21 03:02:05, scherkus wrote: > nit on style: > > render_loop->PostTask(FROM_HERE, base::Bind( > ____&WebMediaPlayerAndroid::OnMediaError, webmediplayer_, > ____error_type)); > > here + below Done. http://codereview.chromium.org/10073016/diff/7001/webkit/media/webmediaplayer... File webkit/media/webmediaplayer_util.h (right): http://codereview.chromium.org/10073016/diff/7001/webkit/media/webmediaplayer... webkit/media/webmediaplayer_util.h:16: base::TimeDelta ConvertSecondsToTimestamp(float seconds) { On 2012/04/21 03:02:05, scherkus wrote: > let's get a .cc for this Done.
Removing myself from reviewers since scherkus has it. (let me know if you want me to look anyway)
LGTM w/ nits thanks! http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... File webkit/media/android/webmediaplayer_android.cc (right): http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... webkit/media/android/webmediaplayer_android.cc:45: const float kTemporaryDuration = 100.0f; nit: media code favours file-static over anon namespace http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... webkit/media/android/webmediaplayer_android.cc:70: WebKit::WebTimeRanges new_buffered(static_cast<size_t>(1)); does the complier stop complaining if you replace "static_cast<size_t>(1)" with "1u"? http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... webkit/media/android/webmediaplayer_android.cc:71: buffered_.swap(new_buffered); can we not initialize buffered_ in the initializer list with buffered_(1u) ? http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... File webkit/media/android/webmediaplayer_proxy_android.cc (right): http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... webkit/media/android/webmediaplayer_proxy_android.cc:29: &WebMediaPlayerAndroid::OnMediaError, webmediaplayer_, nit: try to fit all this stuff onto as few lines as possible here + below
http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... File webkit/media/android/webmediaplayer_android.cc (right): http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... webkit/media/android/webmediaplayer_android.cc:45: const float kTemporaryDuration = 100.0f; On 2012/04/25 03:28:37, scherkus wrote: > nit: media code favours file-static over anon namespace Done. http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... webkit/media/android/webmediaplayer_android.cc:70: WebKit::WebTimeRanges new_buffered(static_cast<size_t>(1)); good catch, although I didn't notice the compiler warning though. On 2012/04/25 03:28:37, scherkus wrote: > does the complier stop complaining if you replace "static_cast<size_t>(1)" with > "1u"? http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... webkit/media/android/webmediaplayer_android.cc:71: buffered_.swap(new_buffered); On 2012/04/25 03:28:37, scherkus wrote: > can we not initialize buffered_ in the initializer list with buffered_(1u) ? Done. http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... File webkit/media/android/webmediaplayer_proxy_android.cc (right): http://codereview.chromium.org/10073016/diff/17001/webkit/media/android/webme... webkit/media/android/webmediaplayer_proxy_android.cc:29: &WebMediaPlayerAndroid::OnMediaError, webmediaplayer_, On 2012/04/25 03:28:37, scherkus wrote: > nit: try to fit all this stuff onto as few lines as possible here + below Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/10073016/27001
Change committed as 133971 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
