Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
409793006:
Eliminate MediaPlayer abstraction(paint APIs- chromium changes) (Closed)
|
Created:
6 years, 5 months ago by Srirama Modified:
6 years, 4 months ago Reviewers:
qinmin CC:
chromium-reviews, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionEliminate MediaPlayer abstraction(paint APIs- chromium changes)
BUG=350571
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286443
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Fixed review comments #
Total comments: 1
Patch Set 3 : formatted to limit lines to 80 chars #
Total comments: 5
Patch Set 4 : nits #
Messages
Total messages: 13 (0 generated)
Moved paintOnAndroid() function to chromium side. Is it the right approach? PTAL
On 2014/07/22 16:22:26, Srirama wrote: > Moved paintOnAndroid() function to chromium side. > Is it the right approach? > PTAL qinmin: Please review this. We are trying to get rid of the MediaPlayer abstraction so code in WebMediaPlayerClientImpl needs to be moved. Since you are more familiar with the specific Android requirements around this code, I'd like you to review it. Thanks.
https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:469: SkBitmap& bitmap, align all the params https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:473: { move this to the previous line https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:474: if (!bitmap.getTexture() || bitmap.width() != size.width || bitmap.height() != size.height) { fix all the indent here, chromium uses 2 spaces, not 4 https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:503: if (player_type_ != MEDIA_PLAYER_TYPE_URL && player_type_ != MEDIA_PLAYER_TYPE_MEDIA_SOURCE) there are only 2 enums defined for player type, this if statement is pretty much useless. https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.h:285: bool ensureTextureBackedSkBitmap(GrContext* gr, SkBitmap& bitmap, const blink::WebSize& size, GrSurfaceOrigin origin, GrPixelConfig config); s/ensure/Ensure/ https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.h:453: SkBitmap m_bitmap; chromium doesn't use this naming scheme, use bitmap_ instead. And I am not so sure whether this variable is even needed, since this variable is only used in the paint function.
Updated as per the review comments. PTAL. https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:469: SkBitmap& bitmap, On 2014/07/25 17:01:09, qinmin wrote: > align all the params Done. https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:473: { On 2014/07/25 17:01:09, qinmin wrote: > move this to the previous line Done. https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:474: if (!bitmap.getTexture() || bitmap.width() != size.width || bitmap.height() != size.height) { On 2014/07/25 17:01:09, qinmin wrote: > fix all the indent here, chromium uses 2 spaces, not 4 Done. https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:503: if (player_type_ != MEDIA_PLAYER_TYPE_URL && player_type_ != MEDIA_PLAYER_TYPE_MEDIA_SOURCE) On 2014/07/25 17:01:09, qinmin wrote: > there are only 2 enums defined for player type, this if statement is pretty much > useless. Done. https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.h:285: bool ensureTextureBackedSkBitmap(GrContext* gr, SkBitmap& bitmap, const blink::WebSize& size, GrSurfaceOrigin origin, GrPixelConfig config); On 2014/07/25 17:01:10, qinmin wrote: > s/ensure/Ensure/ Done. https://codereview.chromium.org/409793006/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.h:453: SkBitmap m_bitmap; On 2014/07/25 17:01:10, qinmin wrote: > chromium doesn't use this naming scheme, use bitmap_ instead. And I am not so > sure whether this variable is even needed, since this variable is only used in > the paint function. Done. Changed name to bitmap_ and regarding whether it is needed or not: the memory is being reused, there are proper checks for reusing it in EnsureTextureBackedSkBitmap(). Only when there is a change in width, height the memory is reallocated. Please correct me if i am wrong.
https://codereview.chromium.org/409793006/diff/100001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/409793006/diff/100001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:498: scoped_ptr<blink::WebGraphicsContext3DProvider> provider = scoped_ptr<blink::WebGraphicsContext3DProvider>(blink::Platform::current()->createSharedOffscreenGraphicsContext3DProvider()); chromium has character limit of 80, break offending lines in this file into multiple lines.
Truncated all the lines to 80 chars max. PTAL.
On 2014/07/26 03:15:27, Srirama wrote: > Truncated all the lines to 80 chars max. > PTAL. ping
lgtm % nits https://codereview.chromium.org/409793006/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/409793006/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:500: scoped_ptr<blink::WebGraphicsContext3DProvider>(blink::Platform::current( nit: +2 indent https://codereview.chromium.org/409793006/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:522: return; nit: you need {} if the if statement spans multiple lines
Updated nits. https://codereview.chromium.org/409793006/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/409793006/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:500: scoped_ptr<blink::WebGraphicsContext3DProvider>(blink::Platform::current( On 2014/07/30 05:25:52, qinmin wrote: > nit: +2 indent Done. https://codereview.chromium.org/409793006/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:500: scoped_ptr<blink::WebGraphicsContext3DProvider>(blink::Platform::current( On 2014/07/30 05:25:52, qinmin wrote: > nit: +2 indent Done. https://codereview.chromium.org/409793006/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:522: return; On 2014/07/30 05:25:52, qinmin wrote: > nit: you need {} if the if statement spans multiple lines Done.
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/409793006/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Message was sent while issue was closed.
Change committed as 286443 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
