Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153093003:
WebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap (Closed)
|
Created:
4 years, 5 months ago by mcasas Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap
Tot has ARGB hardcoded, whereas certain platforms (e.g.
Android) usually have ABGR. Make that decision runtime.
Also, code in ToT uses SkBitmap, which is discouraged,
this CL changes it to using SkSurface and SkPixmap
instead.
BUG=628466
Committed: https://crrev.com/8e2816a491e5f15d5fdfc809e7bad74f0cfcb9cb
Cr-Commit-Position: refs/heads/master@{#406086}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : dcastagna@s comments #Patch Set 3 : emircan@s comments #Messages
Total messages: 20 (10 generated)
Description was changed from ========== WebMediaPlayerMsCompositor: take into account if incoming frame is ARGB or ABGR Tot has ARGB hardcoded, whereas certain platforms (e.g. Android) usually have ABGR. Make that decision runtime. BUG=628466 ========== to ========== WebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap Tot has ARGB hardcoded, whereas certain platforms (e.g. Android) usually have ABGR. Make that decision runtime. Also, code in ToT uses SkBitmap, which is discouraged, this CL changes it to using SkSurface and SkPixmap instead. BUG=628466 ==========
Patchset #1 (id:1) has been deleted
mcasas@chromium.org changed reviewers: + dcastagna@chromium.org, emircan@chromium.org
emircan@/dcastagna@ PTAL
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
LGTM with nits and questions. https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms_compositor.cc (right): https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:50: // TODO(mcasas): Reuse |surface| (as long as frame->visible_rect() doesn't Why? Is it expensive to create one? https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:57: if (surface && provider) { It seems like surface can be null only if we can't allocate SkPixelRef, should we prefer DCHECKing that? https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:76: static_cast<uint8*>(pixmap.writable_addr(0, 0)), nit: can you use addr instead of writable_addr? https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:77: pixmap.getSize64(), nit: getSafeSize64 seems to be <= than size64, should we use that instead?
https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms_compositor.cc (right): https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:50: // TODO(mcasas): Reuse |surface| (as long as frame->visible_rect() doesn't On 2016/07/18 16:51:23, Daniele Castagna wrote: > Why? Is it expensive to create one? It boils down to creating a Raster N32 memory, which is actually not needed on a per-frame basis, since we want to use it here as an intermediate whiteboard between |frame| and |new_frame|. I think it can be quite expensive to create, the question would be if SkSurfaces are recycled under the hood. https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:57: if (surface && provider) { On 2016/07/18 16:51:23, Daniele Castagna wrote: > It seems like surface can be null only if we can't allocate SkPixelRef, should > we prefer DCHECKing that? It could also be null due to incorrect width/height. I followed the pattern of e.g. [1,2] of testing |surface|. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Ima... [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imageb... https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:76: static_cast<uint8*>(pixmap.writable_addr(0, 0)), On 2016/07/18 16:51:23, Daniele Castagna wrote: > nit: can you use addr instead of writable_addr? Good catch, done https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:77: pixmap.getSize64(), On 2016/07/18 16:51:23, Daniele Castagna wrote: > nit: getSafeSize64 seems to be <= than size64, should we use that instead? Done.
LGTM
lgtm % the visible_rect changes below. https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms_compositor.cc (right): https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:50: // TODO(mcasas): Reuse |surface| (as long as frame->visible_rect() doesn't On 2016/07/18 17:40:42, mcasas wrote: > On 2016/07/18 16:51:23, Daniele Castagna wrote: > > Why? Is it expensive to create one? > > It boils down to creating a Raster N32 memory, which is > actually not needed on a per-frame basis, since we want > to use it here as an intermediate whiteboard between |frame| > and |new_frame|. I think it can be quite expensive to create, > the question would be if SkSurfaces are recycled under > the hood. Since the use-case here is for pausing media stream, which clearly wouldn't happen per frame, I think it would be OK allocate and deallocate buffers. I dont think we need to hold onto surface buffers when it isn't paused. https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:78: new_frame->data(media::VideoFrame::kYPlane), Consider the case for coded_size() is different than visible_rect() -it can only be bigger-. We allocate SkSurface with visible_rect() on l.53. We need to copy into into visible_data() on l.78, 80, 82. Also, we should change to visible_rect() on l.85 because coded_size() can be bigger than visible_Rect().
https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms_compositor.cc (right): https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:50: // TODO(mcasas): Reuse |surface| (as long as frame->visible_rect() doesn't On 2016/07/18 18:24:25, emircan wrote: > On 2016/07/18 17:40:42, mcasas wrote: > > On 2016/07/18 16:51:23, Daniele Castagna wrote: > > > Why? Is it expensive to create one? > > > > It boils down to creating a Raster N32 memory, which is > > actually not needed on a per-frame basis, since we want > > to use it here as an intermediate whiteboard between |frame| > > and |new_frame|. I think it can be quite expensive to create, > > the question would be if SkSurfaces are recycled under > > the hood. > > Since the use-case here is for pausing media stream, which clearly wouldn't > happen per frame, I think it would be OK allocate and deallocate buffers. I dont > think we need to hold onto surface buffers when it isn't paused. Oh gotcha, removing the TODO then. https://codereview.chromium.org/2153093003/diff/80001/content/renderer/media/... content/renderer/media/webmediaplayer_ms_compositor.cc:78: new_frame->data(media::VideoFrame::kYPlane), On 2016/07/18 18:24:25, emircan wrote: > Consider the case for coded_size() is different than visible_rect() -it can only > be bigger-. We allocate SkSurface with visible_rect() on l.53. We need to copy > into into visible_data() on l.78, 80, 82. Also, we should change to > visible_rect() on l.85 because coded_size() can be bigger than visible_Rect(). Done.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, dcastagna@chromium.org Link to the patchset: https://codereview.chromium.org/2153093003/#ps120001 (title: "emircan@s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== WebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap Tot has ARGB hardcoded, whereas certain platforms (e.g. Android) usually have ABGR. Make that decision runtime. Also, code in ToT uses SkBitmap, which is discouraged, this CL changes it to using SkSurface and SkPixmap instead. BUG=628466 ========== to ========== WebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap Tot has ARGB hardcoded, whereas certain platforms (e.g. Android) usually have ABGR. Make that decision runtime. Also, code in ToT uses SkBitmap, which is discouraged, this CL changes it to using SkSurface and SkPixmap instead. BUG=628466 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== WebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap Tot has ARGB hardcoded, whereas certain platforms (e.g. Android) usually have ABGR. Make that decision runtime. Also, code in ToT uses SkBitmap, which is discouraged, this CL changes it to using SkSurface and SkPixmap instead. BUG=628466 ========== to ========== WebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap Tot has ARGB hardcoded, whereas certain platforms (e.g. Android) usually have ABGR. Make that decision runtime. Also, code in ToT uses SkBitmap, which is discouraged, this CL changes it to using SkSurface and SkPixmap instead. BUG=628466 Committed: https://crrev.com/8e2816a491e5f15d5fdfc809e7bad74f0cfcb9cb Cr-Commit-Position: refs/heads/master@{#406086} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8e2816a491e5f15d5fdfc809e7bad74f0cfcb9cb Cr-Commit-Position: refs/heads/master@{#406086} |
