Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538853002:
Media Remoting: Draw remoting interstitial on poster image. (Closed)
|
Created:
4 years ago by xjz Modified:
3 years, 11 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, chromoting-reviews_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, apacible+watch_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: Draw remoting interstitial on poster image.
Download the poster image when it is available and draw remoting
interstial on it. Changes in this CL:
1. Refactored ImageDownloaderImpl and abstracted the core parts that
does the image downloading to ImageDownloaderBase. There should be no
behaviour change for ImageDownloaderImpl.
2. Added SingleImageDownloader which is a one time image downloader
working in Renderer to return a single image, and can be
self-destructed when downloading finishes.
3. RemotingRendererController will initiate the downloading when the
interstitial needs to be shown if poster image URL changes.
BUG=643964
Committed: https://crrev.com/c102fd8444b2cde0699f0a9a5b812e2d9a923d85
Cr-Commit-Position: refs/heads/master@{#441448}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressed comments. #
Total comments: 37
Patch Set 3 : Addressed comments. #Patch Set 4 : Don't delete in ImageDownloaderBase when RenderFrame is destructed. #Patch Set 5 : Removed the change to update/show interstial from this CL. #
Total comments: 2
Patch Set 6 : Addressed xhwang's comment. #
Total comments: 22
Patch Set 7 : Addressed comments. #
Total comments: 4
Patch Set 8 : Addressed miu's comments. #
Total comments: 7
Patch Set 9 : Rebased only. #Patch Set 10 : Rebased again and Merged. #Patch Set 11 : Use base::<Optional> to tell that the background is unchanged. #
Total comments: 6
Patch Set 12 : Addressed nasko's comments. #Messages
Total messages: 49 (23 generated)
Description was changed from ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when available and draw remoting interstial on it. BUG=643964 ========== to ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when available and draw remoting interstial on it. BUG=643964 ==========
xjz@chromium.org changed reviewers: + miu@chromium.org
PTAL
I'm wondering if we're hacking at ImageDownloaderImpl and RenderFrameImpl too much to make this work? There are a lot of things ImageDownloaderImpl does that we do not want: 1) Mojo, 2) image resizing, etc. Suggestions (both assume RenderFrameImpl shouldn't own a "downloader" for the long-term): 1. Factor-out the common functionality of ImageDownloaderImpl into a base class. Then, have two subclasses: One as a Mojo service (i.e., what it does now), and another as a one-off image downloader that will self-destruct once the download is complete. 2. We could use MultiResolutionImageResourceFetcher directly, and write a static function that manages one-off image downloading. https://codereview.chromium.org/2538853002/diff/1/content/renderer/image_down... File content/renderer/image_downloader/image_downloader_impl.cc (right): https://codereview.chromium.org/2538853002/diff/1/content/renderer/image_down... content/renderer/image_downloader/image_downloader_impl.cc:124: ImageDownloaderImpl::ImageDownloaderImpl( Factoring: This other constructor should delegate to your new ctor, rather that repeat the common initialization. Meaning: ImageDownloaderImpl::ImageDownloaderImpl(...) : ImageDownloaderImpl(render_frame) { binding_.Bind(request); binding_.set_connection_error_handler(...); } https://codereview.chromium.org/2538853002/diff/1/content/renderer/image_down... File content/renderer/image_downloader/image_downloader_impl.h (right): https://codereview.chromium.org/2538853002/diff/1/content/renderer/image_down... content/renderer/image_downloader/image_downloader_impl.h:35: ImageDownloaderImpl(RenderFrame* render_frame); Need explicit keyword here. https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:6680: url, false, 1920 * 1920, false, 1920*1920 seems arbitrary, and some poster images could be very high res (4K or more?). Since you're handling the down-scaling yourself, and so you really don't want ImageDownloaderImpl to do anything here, how about making this std::numeric_limits<uint32_t>::max()? https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.h:1081: void DownloadPoster( These methods bring specific functionality (downloading poster images) into RenderFrameImpl, whose purpose is to own and glue a massive object graph together. Therefore, these need to go somewhere else. See other comments... https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.h:1245: std::unique_ptr<ImageDownloaderImpl> poster_image_downloader_; Looks like ImageDownloaderImpl deletes itself when its OnDestruct() method is called. So, RenderFrameImpl should not own it. https://codereview.chromium.org/2538853002/diff/1/media/base/media_observer.h File media/base/media_observer.h (right): https://codereview.chromium.org/2538853002/diff/1/media/base/media_observer.h... media/base/media_observer.h:31: virtual void OnSetPoster(const GURL& poster) = 0; Please add comment: When is this called? More than once? Why? https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:37: return skia::ImageOperations::Resize( This will stretch (not preserving aspect ratio). Is that okay? Or, should we be adding some cropping logic too? To answer this, you should find out what happens for normal videos by writing some simple HTML and running it in a browser: If the poster image does not match the size or aspect ratio of the video, how is it displayed? Or, even better, maybe this is discussed in the W3C spec? https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:137: DisplayInterstitial(SkBitmap(), is_remoting_successful); Instead of a blank bitmap, it's possible the poster image was already downloaded via a prior call to ShowInterstitial(). So, perhaps you should cache it? You would only provide a black background and re-download the poster image when the poster URL has changed. See comments in controller .cc file... https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:147: weak_factory_.GetWeakPtr(), is_remoting_successful)); What if |is_remoting_successful| changes before this callback is run? (Meaning, what if ShowInterstitial() is called again in the meantime?) I don't think you should pass |is_remoting_successful| to DisplayInterstitial(). Instead, DisplayInterstitial() should check for that when it is called later. https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... File media/remoting/remoting_interstitial_ui.h (right): https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.h:35: void PosterDownloaded(bool is_remoting_successful, See comments in remoting_renderer_controller.h/.cc: The act of downloading and deciding which content to show should be in the controller class. Then, the controller should just always call the existing ShowInterstitial() method like it did before, but now ShowInterstitial() should take two arguments: 1) whether to show the success or fail screen; 2) the SkBitmap background. https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.cc:78: poster_ = poster; See Model-View-Controller comment in header file. Then, here we want to: 1. If poster image URL changed, initiate download. The download callback will then check the URL again (to make sure it is the same) and then pass the resulting SkBitmap to the UI. 2. If poster image URL has become empty, set the empty SkBitmap(). https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.h:79: GURL poster() const { Rather than exposing poster() and download_poster_cb(), let's have the controller be responsible for handling all this, and just give the UI the SkBitmap it needs whenever the poster URL changes. Some quick reading: https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller (In our case, the model and controller are merged together as RemotingRendererController, and that's okay at this scale.)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when available and draw remoting interstial on it. BUG=643964 ========== to ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when it is available and draw remoting interstial on it. Changes in this CL: 1. Refactored ImageDownloadImpl and abstracted the core parts that does the image downloading. 2. Added SingleImageDownloader which is a one time image downloader working in Renderer to return a single image, and can be self-destructed when downloading finishes or render frame is gone. 3. When poster image URL changes, RemotingRendererController will initiate the downloading when the interstitial needs to be shown. BUG=643964 ==========
Thanks miu. Addressed you comments. PTAL. Thanks! Main changes in PS#2: 1. Abstract the core parts of the image downloading to ImageDownloaderCore. 2. Added SingleImageDownloader which is a one time image downloader working in Renderer. 3. Let RemotingRendererController handle the downloading of the poster image, and cache it for future use. https://codereview.chromium.org/2538853002/diff/1/content/renderer/image_down... File content/renderer/image_downloader/image_downloader_impl.cc (right): https://codereview.chromium.org/2538853002/diff/1/content/renderer/image_down... content/renderer/image_downloader/image_downloader_impl.cc:124: ImageDownloaderImpl::ImageDownloaderImpl( On 2016/11/29 22:56:08, miu wrote: > Factoring: This other constructor should delegate to your new ctor, rather that > repeat the common initialization. Meaning: > > ImageDownloaderImpl::ImageDownloaderImpl(...) > : ImageDownloaderImpl(render_frame) { > binding_.Bind(request); > binding_.set_connection_error_handler(...); > } Not applicable. https://codereview.chromium.org/2538853002/diff/1/content/renderer/image_down... File content/renderer/image_downloader/image_downloader_impl.h (right): https://codereview.chromium.org/2538853002/diff/1/content/renderer/image_down... content/renderer/image_downloader/image_downloader_impl.h:35: ImageDownloaderImpl(RenderFrame* render_frame); On 2016/11/29 22:56:08, miu wrote: > Need explicit keyword here. Not applicable. https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:6680: url, false, 1920 * 1920, false, On 2016/11/29 22:56:08, miu wrote: > 1920*1920 seems arbitrary, and some poster images could be very high res (4K or > more?). Since you're handling the down-scaling yourself, and so you really don't > want ImageDownloaderImpl to do anything here, how about making this > std::numeric_limits<uint32_t>::max()? Not applicable. https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.h:1081: void DownloadPoster( On 2016/11/29 22:56:08, miu wrote: > These methods bring specific functionality (downloading poster images) into > RenderFrameImpl, whose purpose is to own and glue a massive object graph > together. Therefore, these need to go somewhere else. See other comments... Done. https://codereview.chromium.org/2538853002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.h:1245: std::unique_ptr<ImageDownloaderImpl> poster_image_downloader_; On 2016/11/29 22:56:08, miu wrote: > Looks like ImageDownloaderImpl deletes itself when its OnDestruct() method is > called. So, RenderFrameImpl should not own it. Not applicable. https://codereview.chromium.org/2538853002/diff/1/media/base/media_observer.h File media/base/media_observer.h (right): https://codereview.chromium.org/2538853002/diff/1/media/base/media_observer.h... media/base/media_observer.h:31: virtual void OnSetPoster(const GURL& poster) = 0; On 2016/11/29 22:56:08, miu wrote: > Please add comment: When is this called? More than once? Why? Done. https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:37: return skia::ImageOperations::Resize( On 2016/11/29 22:56:08, miu wrote: > This will stretch (not preserving aspect ratio). Is that okay? Or, should we be > adding some cropping logic too? To answer this, you should find out what happens > for normal videos by writing some simple HTML and running it in a browser: If > the poster image does not match the size or aspect ratio of the video, how is it > displayed? Or, even better, maybe this is discussed in the W3C spec? Didn't find any specific in the W3C spec. But with some testing in browser, seems that poster image is kept its aspect ratio and is displayed in center if its aspect ratio is not same with that of video. Made the change accordingly. https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:137: DisplayInterstitial(SkBitmap(), is_remoting_successful); On 2016/11/29 22:56:08, miu wrote: > Instead of a blank bitmap, it's possible the poster image was already downloaded > via a prior call to ShowInterstitial(). So, perhaps you should cache it? You > would only provide a black background and re-download the poster image when the > poster URL has changed. See comments in controller .cc file... Done. https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:147: weak_factory_.GetWeakPtr(), is_remoting_successful)); On 2016/11/29 22:56:08, miu wrote: > What if |is_remoting_successful| changes before this callback is run? (Meaning, > what if ShowInterstitial() is called again in the meantime?) > > I don't think you should pass |is_remoting_successful| to DisplayInterstitial(). > Instead, DisplayInterstitial() should check for that when it is called later. Done. https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... File media/remoting/remoting_interstitial_ui.h (right): https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.h:35: void PosterDownloaded(bool is_remoting_successful, On 2016/11/29 22:56:08, miu wrote: > See comments in remoting_renderer_controller.h/.cc: The act of downloading and > deciding which content to show should be in the controller class. Then, the > controller should just always call the existing ShowInterstitial() method like > it did before, but now ShowInterstitial() should take two arguments: 1) whether > to show the success or fail screen; 2) the SkBitmap background. Done. https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.cc:78: poster_ = poster; On 2016/11/29 22:56:08, miu wrote: > See Model-View-Controller comment in header file. Then, here we want to: > > 1. If poster image URL changed, initiate download. The download callback will > then check the URL again (to make sure it is the same) and then pass the > resulting SkBitmap to the UI. > > 2. If poster image URL has become empty, set the empty SkBitmap(). Done. https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2538853002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.h:79: GURL poster() const { On 2016/11/29 22:56:09, miu wrote: > Rather than exposing poster() and download_poster_cb(), let's have the > controller be responsible for handling all this, and just give the UI the > SkBitmap it needs whenever the poster URL changes. > > Some quick reading: > https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller (In our > case, the model and controller are merged together as > RemotingRendererController, and that's okay at this scale.) Done.
Comments on PS2: https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_core.cc (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_core.cc:50: : render_frame_(render_frame) { It makes sense to use the RenderFrameObserver in this base class and override OnDestruct() so that it knows when the |render_frame_| pointer is no longer valid. This could happen, for example, if this class is constructed, but FetchImage() is called at some later point after the RenderFrame is destroyed. Also, unlike the original impl, OnDestruct() would *not* "delete this" since ownership semantics are the responsibility of the subclasses. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_core.h (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_core.h:42: // Requests to fetch an image. When done, the ImageDownloaderImpl s/ImageDownloaderImpl/image downloader/ https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_impl.cc (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_impl.cc:108: // Owns itself. Will be deleted when message pipe is destroyed or render frame This isn't correct: The class doesn't use mojo::StrongBinding, so it will not be destroyed when the message pipe is destroyed. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_impl.h (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_impl.h:58: ImageDownloaderCore image_downloader_; If the core installs a RenderFrameObserver (see other comment), then perhaps this Impl class should subclass the Core class instead, and override OnDestruct() to "delete this": void OnDestruct() { ImageDownloaderCore::OnDestruct(); delete this; } https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/single_image_downloader.cc (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:17: if (cb.is_null()) Should be DCHECK(). https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:21: cb.Run(SkBitmap()); ditto: Should be DCHECK(). https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:50: callback.Run(images[0]); Can the |images| vector ever be empty? How about: callback.Run(images.empty() ? SkBitmap() : images[0]); https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:55: delete this; This second path-to-delete is dangerous and could result in duplicate deletes. Instead, we should make sure the Core class is guaranteed to ALWAYS run the DownloadCallback (which calls DidDownloadCallback()). So, if the RenderFrame is destroyed during a download, the Core class would run the callback with an empty SkBitmap() to indicate the download was aborted. Note: If you make this change, then you no longer need this class to inherit from RenderFrameObserver. After that, there's no reason to instantiate this class at all: Your static public function becomes: // static void SingleImageDownloader::DownloadImageAt(RenderFrame* render_frame, const GURL& url, const base::Callback<void(const SkBitmap&)& callback) { ...DCHECKs here... std::unique_ptr<ImageDowloaderCore> core(new ...); ImageDownloaderCore* core_ptr = core.get(); core_ptr->DownloadImage(..., base::Bind(DidDownloadImage, base::Passed(&core), callback)); } and then the private DidDownloadImage() becomes static: // static void DidDownloadImage(std::unique_ptr<ImageDownloaderCore> to_be_deleted, ...) { callback.Run(...); } https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:122: DCHECK(!canvas_size.IsEmpty()); I'm trying to remember if this can happen. Maybe a web page could, legally, set the video element's width/height to 0x0. So, should this DCHECK be replaced with: if (canvas_size.IsEmpty()) return; https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:28: SkBitmap ResizeImage(const SkBitmap& image, const gfx::Size& canvas_size) { This is view, not controller functionality. It should be moved to the interstitial_ui.h/.cc module. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:29: DCHECK(!canvas_size.IsEmpty()); ditto here: Maybe this should be: if (canvas_size.IsEmpty()) return SkBitmap(); https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:116: poster_ = poster; If the URL has changed, we need to re-render a new interstitial to update the background. (See large comment below.) https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:298: if (!video_renderer_sink) Please add: DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:318: DCHECK(poster_image_.drawsNothing()); ditto: Please add DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:321: const gfx::Size canvas_size = GetRotatedVideoSize( The rest of this method is view, not controller functionality. Stepping back, the design requirements are to always download the new poster image, from the cached source via SingleImageDownloader, and then render a new interstitial to the VideoRendererSink whenever any of these events occurs: 1. The RemotingRendererImpl is constructed (initial rendering). 2. Any time the poster URL changes (to update the background image). 3. Any time the success/fail state of the remoting session changes (to update the status message in the interstitial). 4. Any time the size of the canvas changes (because the scaling of the background and position of the status message will be different). The controller is what receives the poster URL updates, and knows whenever the state of the media remoting session changes. So, it seems logical for the controller to always be the entity to decide when to update the interstitial. The only thing left that we don't have yet: The RemotingRendererImpl needs to notify the controller whenever the canvas size changes. Once that's settled, the controller should have an OnInterstitialNeedsUpdate() method that is called whenever any of #1-#4 happen. This method would: 1) download the poster image; and 2) upon receipt of the SkBitmap from the downloader, pass the bitmap to RemotingRendererImpl to update its view. Then, RemotingRendererImpl would take this SkBitmap, pass it to the function that renders everything, and finally call VideoRendererSink::PaintSingleFrame(). https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.h:132: GURL poster_; naming nit: poster_url_ https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.h:135: SkBitmap poster_image_; Per our face-to-face discussion, you shouldn't need to cache this. Even if we already did the work to scale the image, we don't expect the interstitial to change so often that it would be expensive to download-from-cache and scale it again.
Patchset #3 (id:60001) has been deleted
Addressed comments. PTAL https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_core.cc (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_core.cc:50: : render_frame_(render_frame) { On 2016/12/03 01:05:30, miu wrote: > It makes sense to use the RenderFrameObserver in this base class and override > OnDestruct() so that it knows when the |render_frame_| pointer is no longer > valid. This could happen, for example, if this class is constructed, but > FetchImage() is called at some later point after the RenderFrame is destroyed. > Also, unlike the original impl, OnDestruct() would *not* "delete this" since > ownership semantics are the responsibility of the subclasses. Done. I do "delete this" when OnDestruct() is called to prevent FetchImage() is further called. This will not cause the "double delete". Please see my reply to the other comment. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_core.h (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_core.h:42: // Requests to fetch an image. When done, the ImageDownloaderImpl On 2016/12/03 01:05:30, miu wrote: > s/ImageDownloaderImpl/image downloader/ Done. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_impl.cc (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_impl.cc:108: // Owns itself. Will be deleted when message pipe is destroyed or render frame On 2016/12/03 01:05:30, miu wrote: > This isn't correct: The class doesn't use mojo::StrongBinding, so it will not be > destroyed when the message pipe is destroyed. As chatted face to face, this is correct. When message pipe is destroyed, OnDestruct() will be called to delete "this" because above codes in Line 95-96. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_impl.h (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_impl.h:58: ImageDownloaderCore image_downloader_; On 2016/12/03 01:05:30, miu wrote: > If the core installs a RenderFrameObserver (see other comment), then perhaps > this Impl class should subclass the Core class instead, and override > OnDestruct() to "delete this": > > void OnDestruct() { > ImageDownloaderCore::OnDestruct(); > delete this; > } Changed to use inheritance and "delete this" in base class when RenderFrame is gone. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/single_image_downloader.cc (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:17: if (cb.is_null()) On 2016/12/03 01:05:31, miu wrote: > Should be DCHECK(). Done. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:21: cb.Run(SkBitmap()); On 2016/12/03 01:05:31, miu wrote: > ditto: Should be DCHECK(). Done. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:50: callback.Run(images[0]); On 2016/12/03 01:05:31, miu wrote: > Can the |images| vector ever be empty? How about: > > callback.Run(images.empty() ? SkBitmap() : images[0]); Done. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:55: delete this; On 2016/12/03 01:05:30, miu wrote: > This second path-to-delete is dangerous and could result in duplicate deletes. > I don't think this will cause duplicated delete. There might be two conditions to delete "this": 1. DidDownloadImage() is called first, "this" is deleted and stopped observing RenderFrame, so OnDestruct() will not be called. 2. RenderFrame is gone, OnDestruct() is called first, "this" is deleted, so |image_downloader_| (core) is deleted, which owns all fetchers in |image_fetchers_|, which are also deleted, and the callback is never be run. . > Instead, we should make sure the Core class is guaranteed to ALWAYS run the > DownloadCallback (which calls DidDownloadCallback()). So, if the RenderFrame is > destroyed during a download, the Core class would run the callback with an empty > SkBitmap() to indicate the download was aborted. I didn't do this way because: 1. When RenderFrame is gone, the ImageDownloaderBase should be destroyed to prevent further calling FetchImage(). If instead call the callback anyway, then each subclass is required to override OnDestruct() or "delete this" in the callback. I think it makes sense to let the ImageDownloaderBase itself handle this. 2. ImageDonwloaderBase then needs to maintain a map with all image fetchers and the corresponding callbacks just for this purpose. > > Note: If you make this change, then you no longer need this class to inherit > from RenderFrameObserver. After that, there's no reason to instantiate this > class at all: Your static public function becomes: > > // static > void SingleImageDownloader::DownloadImageAt(RenderFrame* render_frame, > const GURL& url, > const base::Callback<void(const > SkBitmap&)& callback) { > ...DCHECKs here... > > std::unique_ptr<ImageDowloaderCore> core(new ...); > ImageDownloaderCore* core_ptr = core.get(); > core_ptr->DownloadImage(..., base::Bind(DidDownloadImage, base::Passed(&core), > callback)); > } > > and then the private DidDownloadImage() becomes static: > > // static > void DidDownloadImage(std::unique_ptr<ImageDownloaderCore> to_be_deleted, ...) { > callback.Run(...); > } https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:122: DCHECK(!canvas_size.IsEmpty()); On 2016/12/03 01:05:31, miu wrote: > I'm trying to remember if this can happen. Maybe a web page could, legally, set > the video element's width/height to 0x0. So, should this DCHECK be replaced > with: > > if (canvas_size.IsEmpty()) > return; Add the check before this function is called. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:28: SkBitmap ResizeImage(const SkBitmap& image, const gfx::Size& canvas_size) { On 2016/12/03 01:05:31, miu wrote: > This is view, not controller functionality. It should be moved to the > interstitial_ui.h/.cc module. Done. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:29: DCHECK(!canvas_size.IsEmpty()); On 2016/12/03 01:05:31, miu wrote: > ditto here: Maybe this should be: > > if (canvas_size.IsEmpty()) > return SkBitmap(); Not applicable now. If canvas_size is empty, we won't download poster image or show anything. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:116: poster_ = poster; On 2016/12/03 01:05:31, miu wrote: > If the URL has changed, we need to re-render a new interstitial to update the > background. (See large comment below.) Done. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:298: if (!video_renderer_sink) On 2016/12/03 01:05:31, miu wrote: > Please add: DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:318: DCHECK(poster_image_.drawsNothing()); On 2016/12/03 01:05:31, miu wrote: > ditto: Please add DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:321: const gfx::Size canvas_size = GetRotatedVideoSize( On 2016/12/03 01:05:31, miu wrote: > The rest of this method is view, not controller functionality. > > Stepping back, the design requirements are to always download the new poster > image, from the cached source via SingleImageDownloader, and then render a new > interstitial to the VideoRendererSink whenever any of these events occurs: > > 1. The RemotingRendererImpl is constructed (initial rendering). > > 2. Any time the poster URL changes (to update the background image). > > 3. Any time the success/fail state of the remoting session changes (to update > the status message in the interstitial). > > 4. Any time the size of the canvas changes (because the scaling of the > background and position of the status message will be different). > > The controller is what receives the poster URL updates, and knows whenever the > state of the media remoting session changes. So, it seems logical for the > controller to always be the entity to decide when to update the interstitial. > The only thing left that we don't have yet: The RemotingRendererImpl needs to > notify the controller whenever the canvas size changes. > > Once that's settled, the controller should have an OnInterstitialNeedsUpdate() > method that is called whenever any of #1-#4 happen. This method would: 1) > download the poster image; and 2) upon receipt of the SkBitmap from the > downloader, pass the bitmap to RemotingRendererImpl to update its view. Then, > RemotingRendererImpl would take this SkBitmap, pass it to the function that > renders everything, and finally call VideoRendererSink::PaintSingleFrame(). Done. Added a callback to RemotingRendererController to draw and show interstitial after poster imaged is downloaded. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.h:132: GURL poster_; On 2016/12/03 01:05:31, miu wrote: > naming nit: poster_url_ Done. https://codereview.chromium.org/2538853002/diff/40001/media/remoting/remoting... media/remoting/remoting_renderer_controller.h:135: SkBitmap poster_image_; On 2016/12/03 01:05:31, miu wrote: > Per our face-to-face discussion, you shouldn't need to cache this. Even if we > already did the work to scale the image, we don't expect the interstitial to > change so often that it would be expensive to download-from-cache and scale it > again. Done.
Make changes in PS#4 after offline discussion. PTAL. Thanks! Now removed "delete this" in ImageDownloaderBase when OnDestructed() is called. And modified SingleImageDownloader as suggested in the original comment. https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_core.cc (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_core.cc:50: : render_frame_(render_frame) { On 2016/12/06 19:50:55, xjz wrote: > On 2016/12/03 01:05:30, miu wrote: > > It makes sense to use the RenderFrameObserver in this base class and override > > OnDestruct() so that it knows when the |render_frame_| pointer is no longer > > valid. This could happen, for example, if this class is constructed, but > > FetchImage() is called at some later point after the RenderFrame is destroyed. > > Also, unlike the original impl, OnDestruct() would *not* "delete this" since > > ownership semantics are the responsibility of the subclasses. > > Done. I do "delete this" when OnDestruct() is called to prevent FetchImage() is > further called. This will not cause the "double delete". Please see my reply to > the other comment. Now removed "delete this" from OnDestruct(), and call callbacks with empty image vector instead. Note: I don't want to keep a copy of all callbacks in this base class, so now this works as follows: when OnDestruct() is called, all fetchers (MultiResolutionImageResourceFetcher) will get notified, and they will call callback here (ImageDownloaderBase::DidFetchImage). https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/image_downloader_impl.h (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/image_downloader_impl.h:58: ImageDownloaderCore image_downloader_; On 2016/12/06 19:50:55, xjz wrote: > On 2016/12/03 01:05:30, miu wrote: > > If the core installs a RenderFrameObserver (see other comment), then perhaps > > this Impl class should subclass the Core class instead, and override > > OnDestruct() to "delete this": > > > > void OnDestruct() { > > ImageDownloaderCore::OnDestruct(); > > delete this; > > } > > Changed to use inheritance and "delete this" in base class when RenderFrame is > gone. As commented elsewhere, removed "delete this" in base class. And override OnDestruct() here, and "delete this". Didn't call ImageDownloaderBase::OnDestruct() though. Is it necessary since the RenderFrame is already destructed? https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... File content/renderer/image_downloader/single_image_downloader.cc (right): https://codereview.chromium.org/2538853002/diff/40001/content/renderer/image_... content/renderer/image_downloader/single_image_downloader.cc:55: delete this; On 2016/12/06 19:50:55, xjz wrote: > On 2016/12/03 01:05:30, miu wrote: > > This second path-to-delete is dangerous and could result in duplicate deletes. > > > > I don't think this will cause duplicated delete. There might be two conditions > to delete "this": 1. DidDownloadImage() is called first, "this" is deleted and > stopped observing RenderFrame, so OnDestruct() will not be called. 2. > RenderFrame is gone, OnDestruct() is called first, "this" is deleted, so > |image_downloader_| (core) is deleted, which owns all fetchers in > |image_fetchers_|, which are also deleted, and the callback is never be run. . > > > > Instead, we should make sure the Core class is guaranteed to ALWAYS run the > > DownloadCallback (which calls DidDownloadCallback()). So, if the RenderFrame > is > > destroyed during a download, the Core class would run the callback with an > empty > > SkBitmap() to indicate the download was aborted. > > I didn't do this way because: > 1. When RenderFrame is gone, the ImageDownloaderBase should be destroyed to > prevent further calling FetchImage(). If instead call the callback anyway, then > each subclass is required to override OnDestruct() or "delete this" in the > callback. I think it makes sense to let the ImageDownloaderBase itself handle > this. > 2. ImageDonwloaderBase then needs to maintain a map with all image fetchers and > the corresponding callbacks just for this purpose. > > > > > Note: If you make this change, then you no longer need this class to inherit > > from RenderFrameObserver. After that, there's no reason to instantiate this > > class at all: Your static public function becomes: > > > > // static > > void SingleImageDownloader::DownloadImageAt(RenderFrame* render_frame, > > const GURL& url, > > const base::Callback<void(const > > SkBitmap&)& callback) { > > ...DCHECKs here... > > > > std::unique_ptr<ImageDowloaderCore> core(new ...); > > ImageDownloaderCore* core_ptr = core.get(); > > core_ptr->DownloadImage(..., base::Bind(DidDownloadImage, > base::Passed(&core), > > callback)); > > } > > > > and then the private DidDownloadImage() becomes static: > > > > // static > > void DidDownloadImage(std::unique_ptr<ImageDownloaderCore> to_be_deleted, ...) > { > > callback.Run(...); > > } > > Now after making ImageDownloaderBase run callbacks when RenderFrame is destructed, changed SingleImageDownloader as suggested in the original comment.
miu: I moved the control of interstitial update out of this CL, and make a new CL for it (https://codereview.chromium.org/2566223005/). Please hold off reviewing on this one for now. Thanks.
miu: I moved the control of interstitial update out of this CL. PTAL. Thanks!
xjz@chromium.org changed reviewers: + xhwang@chromium.org
Add xhwang for media/base and media/blink changes.
lgtm % nit https://codereview.chromium.org/2538853002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2538853002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:194: void setPoster(const blink::WebURL& poster) override; This is not part of WebMediaPlayerDelegate::Observer interface
Description was changed from ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when it is available and draw remoting interstial on it. Changes in this CL: 1. Refactored ImageDownloadImpl and abstracted the core parts that does the image downloading. 2. Added SingleImageDownloader which is a one time image downloader working in Renderer to return a single image, and can be self-destructed when downloading finishes or render frame is gone. 3. When poster image URL changes, RemotingRendererController will initiate the downloading when the interstitial needs to be shown. BUG=643964 ========== to ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when it is available and draw remoting interstial on it. Changes in this CL: 1. Refactored ImageDownloaderImpl and abstracted the core parts that does the image downloading to ImageDownloaderBase. There should be no behaviour change for ImageDownloaderImpl. 2. Added SingleImageDownloader which is a one time image downloader working in Renderer to return a single image, and can be self-destructed when downloading finishes. 3. RemotingRendererController will initiate the downloading when the interstitial needs to be shown if poster image URL changes. BUG=643964 ==========
xjz@chromium.org changed reviewers: + nasko@chromium.org
xjz@chromium.org changed reviewers: - nasko@chromium.org
Thanks xhwang. Addressed your comment.
Comments for PS6: https://codereview.chromium.org/2538853002/diff/140001/content/renderer/fetch... File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc (right): https://codereview.chromium.org/2538853002/diff/140001/content/renderer/fetch... content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:84: void MultiResolutionImageResourceFetcher::OnRenderFrameDestruct() { Nice! I like this solution much better. :) https://codereview.chromium.org/2538853002/diff/140001/content/renderer/image... File content/renderer/image_downloader/single_image_downloader.cc (right): https://codereview.chromium.org/2538853002/diff/140001/content/renderer/image... content/renderer/image_downloader/single_image_downloader.cc:17: DCHECK(render_frame); See comment in header file (re: using WeakPtr instead). This would become: if (!render_frame) { cb.Run(SkBitmap()); return; } https://codereview.chromium.org/2538853002/diff/140001/content/renderer/image... File content/renderer/image_downloader/single_image_downloader.h (right): https://codereview.chromium.org/2538853002/diff/140001/content/renderer/image... content/renderer/image_downloader/single_image_downloader.h:23: static void DownloadImage(RenderFrame* render_frame, Because the RenderFrame is used in a callback function, it feels like this should be a WeakPtr<RenderFrame> here. (Just in case there are any weird shutdown race conditions.) Then, inside this function, if the render_frame.get() is null, run the DownloadImageCallback with an empty SkBitmap. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:81: UpdateAndMaybeShowInterstitial(); Instead of calling UpdateInterstitial() here, I would suggest you just run the download callback here, and then let PosterImageDownloaded() decide whether to call UpdateInterstitial() later. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:262: void RemotingRendererController::UpdateAndMaybeShowInterstitial() { naming nit: It's fine to just call this UpdateInterstitial() (This'll be a merge task after your other CL lands.) https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:275: weak_factory_.GetWeakPtr())); Note: Recommend you pass |poster_url_| as an argument here. (See comment below for why.) https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:279: void RemotingRendererController::PosterImageDownloaded(const SkBitmap& image) { naming nit: OnPosterImageDownloaded(). https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:279: void RemotingRendererController::PosterImageDownloaded(const SkBitmap& image) { Recommend you double-check that the Poster URL has not changed in the meantime. If it has, just discard the result. Something like: void RemotingRendererController::PosterImageDownloaded(const GURL& download_url, const SkBitmap& image) { DCHECK(thread_checker_.CalledOnValidThread()); if (download_url != poster_url_) { // The poster image URL has changed during the download. return; } poster_image_ = image; UpdateInterstitial(); } https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:281: DCHECK(remote_rendering_started_); Since the downloading can take an unbounded amount of time, it's easily possible that remote rendering was stopped during the download. That shouldn't crash the process, so I suggest removing this DCHECK(). Just let the call to UpdateInterstitial() decide what to do. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.h:141: GURL poster_url_; nit: Needs comment ("Current poster URL, whose image will feed into the local UI.") https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.h:143: DownloadPosterCallback download_poster_cb_; ditto: Short comment, please.
Addressed comments. PTAL. https://codereview.chromium.org/2538853002/diff/140001/content/renderer/fetch... File content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc (right): https://codereview.chromium.org/2538853002/diff/140001/content/renderer/fetch... content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc:84: void MultiResolutionImageResourceFetcher::OnRenderFrameDestruct() { On 2016/12/20 00:48:06, miu wrote: > Nice! I like this solution much better. :) Thanks. :) https://codereview.chromium.org/2538853002/diff/140001/content/renderer/image... File content/renderer/image_downloader/single_image_downloader.cc (right): https://codereview.chromium.org/2538853002/diff/140001/content/renderer/image... content/renderer/image_downloader/single_image_downloader.cc:17: DCHECK(render_frame); On 2016/12/20 00:48:06, miu wrote: > See comment in header file (re: using WeakPtr instead). This would become: > > if (!render_frame) { > cb.Run(SkBitmap()); > return; > } Done. https://codereview.chromium.org/2538853002/diff/140001/content/renderer/image... File content/renderer/image_downloader/single_image_downloader.h (right): https://codereview.chromium.org/2538853002/diff/140001/content/renderer/image... content/renderer/image_downloader/single_image_downloader.h:23: static void DownloadImage(RenderFrame* render_frame, On 2016/12/20 00:48:06, miu wrote: > Because the RenderFrame is used in a callback function, it feels like this > should be a WeakPtr<RenderFrame> here. (Just in case there are any weird > shutdown race conditions.) Then, inside this function, if the render_frame.get() > is null, run the DownloadImageCallback with an empty SkBitmap. Done. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:81: UpdateAndMaybeShowInterstitial(); On 2016/12/20 00:48:06, miu wrote: > Instead of calling UpdateInterstitial() here, I would suggest you just run the > download callback here, and then let PosterImageDownloaded() decide whether to > call UpdateInterstitial() later. Done. The poster image will be downloaded here only when url changes and interstitial callback is set (after merging with another CL). After merge, it will be also downloaded when the interstitial callback is set (if the url was set before). https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:262: void RemotingRendererController::UpdateAndMaybeShowInterstitial() { On 2016/12/20 00:48:06, miu wrote: > naming nit: It's fine to just call this UpdateInterstitial() (This'll be a merge > task after your other CL lands.) Done. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:275: weak_factory_.GetWeakPtr())); On 2016/12/20 00:48:06, miu wrote: > Note: Recommend you pass |poster_url_| as an argument here. (See comment below > for why.) Done. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:279: void RemotingRendererController::PosterImageDownloaded(const SkBitmap& image) { On 2016/12/20 00:48:06, miu wrote: > naming nit: OnPosterImageDownloaded(). Done. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:279: void RemotingRendererController::PosterImageDownloaded(const SkBitmap& image) { On 2016/12/20 00:48:06, miu wrote: > Recommend you double-check that the Poster URL has not changed in the meantime. > If it has, just discard the result. Something like: > > void RemotingRendererController::PosterImageDownloaded(const GURL& download_url, > const SkBitmap& image) { > DCHECK(thread_checker_.CalledOnValidThread()); > if (download_url != poster_url_) { > // The poster image URL has changed during the download. > return; > } > poster_image_ = image; > UpdateInterstitial(); > } Done. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:281: DCHECK(remote_rendering_started_); On 2016/12/20 00:48:06, miu wrote: > Since the downloading can take an unbounded amount of time, it's easily possible > that remote rendering was stopped during the download. That shouldn't crash the > process, so I suggest removing this DCHECK(). Just let the call to > UpdateInterstitial() decide what to do. Done. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.h:141: GURL poster_url_; On 2016/12/20 00:48:07, miu wrote: > nit: Needs comment ("Current poster URL, whose image will feed into the local > UI.") Done. https://codereview.chromium.org/2538853002/diff/140001/media/remoting/remotin... media/remoting/remoting_renderer_controller.h:143: DownloadPosterCallback download_poster_cb_; On 2016/12/20 00:48:06, miu wrote: > ditto: Short comment, please. Done.
PS7 lgtm % a couple minor things: https://codereview.chromium.org/2538853002/diff/160001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/160001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:82: // TODO(xjz): Only download the image when we need to draw/update remoting I'm not sure this is what we would want. It could be good to download the poster image now; and then later, once we *can* draw a remoting interstitial, the poster image will already be downloaded and ready to use. https://codereview.chromium.org/2538853002/diff/160001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:270: download_poster_cb_ = cb; This just occurred to me: What if the poster URL is set *before* this method is called? (It's technically possible, since this method isn't called until after WMPI is created, and WMPI is allowed to call OnSetPoster() beforehand.) I would suggest adding at the end: if (!poster_url_.empty()) download_poster_cb_.Run(...);
miu: Thanks for reviewing. Addressed your comments. https://codereview.chromium.org/2538853002/diff/160001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/160001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:82: // TODO(xjz): Only download the image when we need to draw/update remoting On 2016/12/20 23:34:22, miu wrote: > I'm not sure this is what we would want. It could be good to download the poster > image now; and then later, once we *can* draw a remoting interstitial, the > poster image will already be downloaded and ready to use. hmm, I think we don't want to download and keep a copy of the image for local playback or mirroring. So the downloading will only be done if the interstitial callback is set. This introduces latency to show the poster image as background of interstitial when remoting starts. But the latency is only introduced once. The interstitial will be drawn on black image first and the background is updated later once the downloading completes. Does this sgty? https://codereview.chromium.org/2538853002/diff/160001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:270: download_poster_cb_ = cb; On 2016/12/20 23:34:22, miu wrote: > This just occurred to me: What if the poster URL is set *before* this method is > called? (It's technically possible, since this method isn't called until after > WMPI is created, and WMPI is allowed to call OnSetPoster() beforehand.) > > I would suggest adding at the end: > > if (!poster_url_.empty()) > download_poster_cb_.Run(...); Done. I didn't realize this. Thanks for pointing this out.
xjz@chromium.org changed reviewers: + nasko@chromium.org
nasko: PTAL the changes on content/*. Thanks! Main changes: 1. Refactored the ImageDownloaderImpl. Abstracted the core parts from it to ImageDownloaderBase. There should be no behaviour change to ImageDownloaderImpl. 2. Added a SingleImageDownloader that will be self-destructed after downloading finishes.
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments on PS7: https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:81: DownloadPosterImage(); What if the URL is set to empty (i.e., the web page has cleared the property)? IIUC, we should interpret that to mean that the page is removing the poster image. Granted, this is an edge-case, but it could happen, so we should probably account for that with something like: if (poster_url != poster_url_) { poster_url_ = poster_url; if (poster_url.is_empty()) { UpdateInterstitial(SkBitmap()); } else { DownloadPosterImage(); } } https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:260: DownloadPosterImage(); Along the lines of my previous comment: if (!poster_url_.is_empty()) DownloadPosterImage(); https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:274: if (download_poster_cb_.is_null() || poster_url_.is_empty()) nit: Suggest taking out the "|| poster_url_.is_empty()" part since the decision is made by the caller of this method.
miu: Rebased and merged with the other CL. PTAL again. Thanks! https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:81: DownloadPosterImage(); On 2016/12/22 02:04:02, miu wrote: > What if the URL is set to empty (i.e., the web page has cleared the property)? > IIUC, we should interpret that to mean that the page is removing the poster > image. Granted, this is an edge-case, but it could happen, so we should probably > account for that with something like: > > if (poster_url != poster_url_) { > poster_url_ = poster_url; > if (poster_url.is_empty()) { > UpdateInterstitial(SkBitmap()); > } else { > DownloadPosterImage(); > } > } This case is really an edge case as the poster image is mainly to be shown while video is downloading or before user clicks the play button. It seems no use to clear it after. I think it makes sense to use the previously downloaded image as the background for the interstitial in this edge case. WDYT? The current design is that we will only download every poster image once in each remoting session, and RemoteRendererImpl will keep a copy, and use it to draw remoting interstitial whenever update is needed. So except calling from OnPosterImageDownloaded(), we just simply call UpdateInterstitial(SkBitmap()) to request updating the interstitial. Please see the comment for UpdateInterstitial() in the header file. If we want to handle the above edge case and clear the background image when an empty url is set, we could do either of the following: 1. Keep a copy of the downloaded image in both RemoteRendererImpl and here. 2. Download the image again whenever need to update the interstitial and send it to RemoteRendererImpl. 3. Add another callback (UpdateBackgroudImageCallback) or keep a reference to RemoteRendererImpl (or using an interface) here to update (clear) the background image whenever needed. WDYT?
Addressed miu's comments in PS# 11. PTAL. Thanks! https://codereview.chromium.org/2538853002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2538853002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:194: void setPoster(const blink::WebURL& poster) override; On 2016/12/15 18:35:23, xhwang_OOO_until_Jan3 wrote: > This is not part of WebMediaPlayerDelegate::Observer interface Done in PS #6. https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:81: DownloadPosterImage(); On 2016/12/22 20:13:08, xjz wrote: > On 2016/12/22 02:04:02, miu wrote: > > What if the URL is set to empty (i.e., the web page has cleared the property)? > > IIUC, we should interpret that to mean that the page is removing the poster > > image. Granted, this is an edge-case, but it could happen, so we should > probably > > account for that with something like: > > > > if (poster_url != poster_url_) { > > poster_url_ = poster_url; > > if (poster_url.is_empty()) { > > UpdateInterstitial(SkBitmap()); > > } else { > > DownloadPosterImage(); > > } > > } > > This case is really an edge case as the poster image is mainly to be shown while > video is downloading or before user clicks the play button. It seems no use to > clear it after. I think it makes sense to use the previously downloaded image as > the background for the interstitial in this edge case. WDYT? > > The current design is that we will only download every poster image once in each > remoting session, and RemoteRendererImpl will keep a copy, and use it to draw > remoting interstitial whenever update is needed. So except calling from > OnPosterImageDownloaded(), we just simply call UpdateInterstitial(SkBitmap()) to > request updating the interstitial. Please see the comment for > UpdateInterstitial() in the header file. If we want to handle the above edge > case and clear the background image when an empty url is set, we could do either > of the following: > 1. Keep a copy of the downloaded image in both RemoteRendererImpl and here. > 2. Download the image again whenever need to update the interstitial and send it > to RemoteRendererImpl. > 3. Add another callback (UpdateBackgroudImageCallback) or keep a reference to > RemoteRendererImpl (or using an interface) here to update (clear) the background > image whenever needed. > > WDYT? Done. As chatted face to face, change UpdateInterstitial() argument to base::<Optional>. https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:260: DownloadPosterImage(); On 2016/12/22 02:04:02, miu wrote: > Along the lines of my previous comment: > > if (!poster_url_.is_empty()) > DownloadPosterImage(); Done. https://codereview.chromium.org/2538853002/diff/180001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:274: if (download_poster_cb_.is_null() || poster_url_.is_empty()) On 2016/12/22 02:04:02, miu wrote: > nit: Suggest taking out the "|| poster_url_.is_empty()" part since the decision > is made by the caller of this method. Done.
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping nasko: need OWNER's approval on content/*. PTAL or suggest someone else. Thanks!
PS11 lgtm
LGTM with a few nits. https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... File content/renderer/image_downloader/image_downloader_impl.cc (right): https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... content/renderer/image_downloader/image_downloader_impl.cc:106: // Owns itself. Will be deleted when message pipe is destroyed or render frame nit: s/render frame/RenderFrame/ https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... File content/renderer/image_downloader/single_image_downloader.cc (right): https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... content/renderer/image_downloader/single_image_downloader.cc:33: std::unique_ptr<ImageDownloaderBase> image_downloader, nit: Maybe put a comment as to why image_download is unused. Seem we pass it in as unique_ptr to keep it alive, but it might not be obvious. https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... File content/renderer/image_downloader/single_image_downloader.h (right): https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... content/renderer/image_downloader/single_image_downloader.h:14: // multiple frames, return the first one. Return an empty bitmap if downloading nit: Returns May be worthwhile mentioning that this class does not impose size limitations on the image.
Thanks nasko! Addressed your comments. https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... File content/renderer/image_downloader/image_downloader_impl.cc (right): https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... content/renderer/image_downloader/image_downloader_impl.cc:106: // Owns itself. Will be deleted when message pipe is destroyed or render frame On 2017/01/04 17:45:33, nasko wrote: > nit: s/render frame/RenderFrame/ Done. https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... File content/renderer/image_downloader/single_image_downloader.cc (right): https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... content/renderer/image_downloader/single_image_downloader.cc:33: std::unique_ptr<ImageDownloaderBase> image_downloader, On 2017/01/04 17:45:33, nasko wrote: > nit: Maybe put a comment as to why image_download is unused. Seem we pass it in > as unique_ptr to keep it alive, but it might not be obvious. Done. Yes, this is to keep it alive while downloading and destroy it after downloading finishes. Added comments in header file. https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... File content/renderer/image_downloader/single_image_downloader.h (right): https://codereview.chromium.org/2538853002/diff/240001/content/renderer/image... content/renderer/image_downloader/single_image_downloader.h:14: // multiple frames, return the first one. Return an empty bitmap if downloading On 2017/01/04 17:45:33, nasko wrote: > nit: Returns > May be worthwhile mentioning that this class does not impose size limitations on > the image. Done.
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, miu@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2538853002/#ps260001 (title: "Addressed nasko's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1483555104278660,
"parent_rev": "69bf9e0ed6580b830f61fff18f7dd98f349facc7", "commit_rev":
"29c1b7da08c5a12a98e9031c1f8d188cbd006a5b"}
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when it is available and draw remoting interstial on it. Changes in this CL: 1. Refactored ImageDownloaderImpl and abstracted the core parts that does the image downloading to ImageDownloaderBase. There should be no behaviour change for ImageDownloaderImpl. 2. Added SingleImageDownloader which is a one time image downloader working in Renderer to return a single image, and can be self-destructed when downloading finishes. 3. RemotingRendererController will initiate the downloading when the interstitial needs to be shown if poster image URL changes. BUG=643964 ========== to ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when it is available and draw remoting interstial on it. Changes in this CL: 1. Refactored ImageDownloaderImpl and abstracted the core parts that does the image downloading to ImageDownloaderBase. There should be no behaviour change for ImageDownloaderImpl. 2. Added SingleImageDownloader which is a one time image downloader working in Renderer to return a single image, and can be self-destructed when downloading finishes. 3. RemotingRendererController will initiate the downloading when the interstitial needs to be shown if poster image URL changes. BUG=643964 Review-Url: https://codereview.chromium.org/2538853002 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when it is available and draw remoting interstial on it. Changes in this CL: 1. Refactored ImageDownloaderImpl and abstracted the core parts that does the image downloading to ImageDownloaderBase. There should be no behaviour change for ImageDownloaderImpl. 2. Added SingleImageDownloader which is a one time image downloader working in Renderer to return a single image, and can be self-destructed when downloading finishes. 3. RemotingRendererController will initiate the downloading when the interstitial needs to be shown if poster image URL changes. BUG=643964 Review-Url: https://codereview.chromium.org/2538853002 ========== to ========== Media Remoting: Draw remoting interstitial on poster image. Download the poster image when it is available and draw remoting interstial on it. Changes in this CL: 1. Refactored ImageDownloaderImpl and abstracted the core parts that does the image downloading to ImageDownloaderBase. There should be no behaviour change for ImageDownloaderImpl. 2. Added SingleImageDownloader which is a one time image downloader working in Renderer to return a single image, and can be self-destructed when downloading finishes. 3. RemotingRendererController will initiate the downloading when the interstitial needs to be shown if poster image URL changes. BUG=643964 Committed: https://crrev.com/c102fd8444b2cde0699f0a9a5b812e2d9a923d85 Cr-Commit-Position: refs/heads/master@{#441448} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c102fd8444b2cde0699f0a9a5b812e2d9a923d85 Cr-Commit-Position: refs/heads/master@{#441448} |
