Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567013002:
[MediaNotification] Removing upper limit of MediaImage sizes (Closed)
|
Created:
4 years ago by Zhiqiang Zhang (Slow) Modified:
4 years ago Reviewers:
whywhat CC:
agrieve+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MediaNotification] Removing upper limit of MediaImage sizes
Currently MediaImageManager uses 8x of the ideal size as the upper limit
for downloading image, however images of sizes between 4x~8x will be
rated as 0 and will not be used after they are downloaded.
This CL removes the upper size limit for MediaImage. The upper limit was
used to prevent download huge media images, however ImageDownloader does
not do it for us. As long as other modules (like manifest) don't enforce
upper limit on image sizes for download, it's OK to keep the pattern
consistent in media notification.
BUG=673360
Committed: https://crrev.com/db38286b8461683814d0fe2e6920feb864262968
Cr-Commit-Position: refs/heads/master@{#438315}
Patch Set 1 #Patch Set 2 : made upper limit for downloading and evaluation consistent #Patch Set 3 : removed upper size limit #
Total comments: 6
Patch Set 4 : addressed nits from avayvod@ #Patch Set 5 : addressed findbug error #Messages
Total messages: 22 (14 generated)
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
Description was changed from ========== [MediaNotification] Show artwork after download finished even if it's too large Sometimes the downloaded MediaImage downloaded is too large or too small (when sizes attribute is "any" or mismatches the real size). In this situation we won't use it in the media notification. However in this case the network traffic will be wasted. In this CL, we show the downloaded image neglect of its actual size. BUG=673360 ========== to ========== [MediaNotification] Make upper size limit of MediaImage consitent for downloading and evaluation Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL will make the upper size limit for downloading and evaluation consistent. BUG=673360 ==========
Patchset #2 (id:20001) has been deleted
PTAL wrt. the discussion on crbug :)
Message was sent while issue was closed.
Description was changed from ========== [MediaNotification] Make upper size limit of MediaImage consitent for downloading and evaluation Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL will make the upper size limit for downloading and evaluation consistent. BUG=673360 ========== to ========== [MediaNotification] Removing upper limit of MediaImage sizes Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL removes the upper size limit for MediaImage. The upper limit was used to prevent download huge media images, however ImageDownloader does not do it for us. As long as other modules (like manifest) don't enforce upper limit on image sizes for download, it's OK to keep the pattern consistent. BUG=673360 ==========
Description was changed from ========== [MediaNotification] Removing upper limit of MediaImage sizes Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL removes the upper size limit for MediaImage. The upper limit was used to prevent download huge media images, however ImageDownloader does not do it for us. As long as other modules (like manifest) don't enforce upper limit on image sizes for download, it's OK to keep the pattern consistent. BUG=673360 ========== to ========== [MediaNotification] Removing upper limit of MediaImage sizes Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL removes the upper size limit for MediaImage. The upper limit was used to prevent download huge media images, however ImageDownloader does not do it for us. As long as other modules (like manifest) don't enforce upper limit on image sizes for download, it's OK to keep the pattern consistent in media notification. BUG=673360 ==========
PTAL per latest discussion on crbug This CL removes the upper size limit, and I'll print console logs in a follow-up
lgtm with nits we do still resize the image later to display, right? maybe we should pass the maximum size we show to the fetcher for optimization (in hopes it will resize sooner, more effectively)? https://codereview.chromium.org/2567013002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaImageManager.java (right): https://codereview.chromium.org/2567013002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaImageManager.java:57: private static final double SIZE_FACTOR_UPPER_LIMIT = 8; nit: unused? https://codereview.chromium.org/2567013002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaImageManager.java:135: image.getSrc() /* url */, nit: could you use a slightly different style? image.getSrc(), // url false, // isFavicon Maybe I recall incorrectly but this way you wouldn't violate the two spaces between code and comments style guide rule? https://codereview.chromium.org/2567013002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaImageManager.java:137: 0 /* maxBitmapSize*/, maybe add a comment before the call to explain we don't want to limit the upper size of the image?
> we do still resize the image later to display, right? maybe we should pass the > maximum size we show to the fetcher for optimization (in hopes it will resize > sooner, more effectively)? Actually passing mIdealSize as the upper limit has side effects. If the image downloader get two images of size 0.5*mIdealSize and 2*mIdealSize, it will drop the large one. So now what I'm doing is to just have a device-independent size for that limit (2048px). If only one image is downloaded and it is larger than this limit, it will be scaled twice (by image downloader and by MediaNotificationManager), but at least we can avoid passing and keeping huge bitmaps in Java. https://codereview.chromium.org/2567013002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaImageManager.java (right): https://codereview.chromium.org/2567013002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaImageManager.java:57: private static final double SIZE_FACTOR_UPPER_LIMIT = 8; On 2016/12/13 18:10:51, whywhat wrote: > nit: unused? It is now used in the latest patch on calling DownloadImage() to avoid passing huge bitmaps though JNI. I made it a device-independent value (2048). https://codereview.chromium.org/2567013002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaImageManager.java:135: image.getSrc() /* url */, On 2016/12/13 18:10:51, whywhat wrote: > nit: could you use a slightly different style? > > image.getSrc(), // url > false, // isFavicon > > Maybe I recall incorrectly but this way you wouldn't violate the two spaces > between code and comments style guide rule? Done. https://codereview.chromium.org/2567013002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaImageManager.java:137: 0 /* maxBitmapSize*/, On 2016/12/13 18:10:51, whywhat wrote: > maybe add a comment before the call to explain we don't want to limit the upper > size of the image? Done.
The CQ bit was checked by zqzhang@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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2567013002/#ps100001 (title: "addressed findbug error")
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": 100001, "attempt_start_ts": 1481665295944110,
"parent_rev": "c2417a52c4f582ecb5b154128fad72e09ba2292f", "commit_rev":
"2133131bd0ae64d4fe24a5ab2703b93a034e3e50"}
Message was sent while issue was closed.
Description was changed from ========== [MediaNotification] Removing upper limit of MediaImage sizes Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL removes the upper size limit for MediaImage. The upper limit was used to prevent download huge media images, however ImageDownloader does not do it for us. As long as other modules (like manifest) don't enforce upper limit on image sizes for download, it's OK to keep the pattern consistent in media notification. BUG=673360 ========== to ========== [MediaNotification] Removing upper limit of MediaImage sizes Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL removes the upper size limit for MediaImage. The upper limit was used to prevent download huge media images, however ImageDownloader does not do it for us. As long as other modules (like manifest) don't enforce upper limit on image sizes for download, it's OK to keep the pattern consistent in media notification. BUG=673360 Review-Url: https://codereview.chromium.org/2567013002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [MediaNotification] Removing upper limit of MediaImage sizes Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL removes the upper size limit for MediaImage. The upper limit was used to prevent download huge media images, however ImageDownloader does not do it for us. As long as other modules (like manifest) don't enforce upper limit on image sizes for download, it's OK to keep the pattern consistent in media notification. BUG=673360 Review-Url: https://codereview.chromium.org/2567013002 ========== to ========== [MediaNotification] Removing upper limit of MediaImage sizes Currently MediaImageManager uses 8x of the ideal size as the upper limit for downloading image, however images of sizes between 4x~8x will be rated as 0 and will not be used after they are downloaded. This CL removes the upper size limit for MediaImage. The upper limit was used to prevent download huge media images, however ImageDownloader does not do it for us. As long as other modules (like manifest) don't enforce upper limit on image sizes for download, it's OK to keep the pattern consistent in media notification. BUG=673360 Committed: https://crrev.com/db38286b8461683814d0fe2e6920feb864262968 Cr-Commit-Position: refs/heads/master@{#438315} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/db38286b8461683814d0fe2e6920feb864262968 Cr-Commit-Position: refs/heads/master@{#438315} |
