Implement Raster Backend on Android Viewer App

Implement Raster Backend on Android Viewer App

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041193004

Committed: https://skia.googlesource.com/skia/+/d94ad5823b7da7f115e997d12828314e290981f6

[liyuqian, 2016-06-07 15:32:15]

Description was changed from

==========
Implement Raster Backend on Android Viewer App

BUG=skia:
==========

to

==========
Implement Raster Backend on Android Viewer App

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041193004
==========

[liyuqian, 2016-06-07 15:35:36]

Description was changed from

==========
Implement Raster Backend on Android Viewer App

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041193004
==========

to

==========
Implement Raster Backend on Android Viewer App

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041193004
==========

[liyuqian, 2016-06-07 15:35:37]

liyuqian@google.com changed reviewers:
+ djsollen@google.com, jvanverth@google.com

[djsollen, 2016-06-07 16:05:30]

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
File tools/viewer/sk_app/RasterWindowContext.h (right):

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:24: void resize(uint32_t w, uint32_t
h) override {
why doesn't the android subclass override this and call
ANativeWindow_setBuffersGeometry

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:33: SkDEBUGFAIL("setDisplayParams is
currently unsupported.");
why doesn't the android subclass override this?

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:36: GrBackendContext
getBackendContext() override { return (GrBackendContext) nullptr; }
no need to cast the nullptr

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:47: SkImageInfo getImageInfo();
remove this function.

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:53: sk_sp<SkSurface>
fBackbufferSurface = nullptr;
we shouldn't need to store this.

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/RasterWindowContext_android.cpp (right):

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.cpp:49:
image->readPixels(this->getImageInfo(), buffer.bits, buffer.stride *
bytePerPixel, 0, 0);
we should not do a pixel copy on every frame.  The getBackBufferSurface() should
be implemented here and when it is called it needs to lock the window.  This
call should just unlock and post the buffer that was locked.

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (left):

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:53:
this->initDisplay(fNativeWindow);
is fNativeWindow always false?

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:68: fWindowContext = new
RasterWindowContext_android((void*)&platformData, fDisplayParams);
this should use the same factory creation as the other backends and look like
RasterWindowContext::Create(...)

[liyuqian, 2016-06-07 19:19:58]

MakeRasterDirect magically works now. I guess it's due to the recreation of
SurfaceView. I'm also deleting RasterWindowContext since it's not saving
anything.

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
File tools/viewer/sk_app/RasterWindowContext.h (right):

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:24: void resize(uint32_t w, uint32_t
h) override {
On 2016/06/07 16:05:29, djsollen wrote:
> why doesn't the android subclass override this and call
> ANativeWindow_setBuffersGeometry

I'm deleting RasterWindowContext since there isn't much common code in the
RasterWindowContext that can be applied to each platform.

Previously, I did not override this in Android because we never call resize in
Android.

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:33: SkDEBUGFAIL("setDisplayParams is
currently unsupported.");
On 2016/06/07 16:05:29, djsollen wrote:
> why doesn't the android subclass override this?

I'm deleting RasterWindowContext since there isn't much common code in the
RasterWindowContext that can be applied to each platform.

Previously, I did not override this in Android because we never call
setDisplayParams in
Android.

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:36: GrBackendContext
getBackendContext() override { return (GrBackendContext) nullptr; }
On 2016/06/07 16:05:29, djsollen wrote:
> no need to cast the nullptr

If not, it won't compile...

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:47: SkImageInfo getImageInfo();
On 2016/06/07 16:05:29, djsollen wrote:
> remove this function.

Removed. Previously, we need this because of some very strange issues...

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/Ras...
tools/viewer/sk_app/RasterWindowContext.h:53: sk_sp<SkSurface>
fBackbufferSurface = nullptr;
On 2016/06/07 16:05:29, djsollen wrote:
> we shouldn't need to store this.

We need to store this because one might call getBackbufferSurface multiple times
before swapBuffers.

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/RasterWindowContext_android.cpp (right):

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.cpp:49:
image->readPixels(this->getImageInfo(), buffer.bits, buffer.stride *
bytePerPixel, 0, 0);
On 2016/06/07 16:05:30, djsollen wrote:
> we should not do a pixel copy on every frame.  The getBackBufferSurface()
should
> be implemented here and when it is called it needs to lock the window.  This
> call should just unlock and post the buffer that was locked.

Done.

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2041193004/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:68: fWindowContext = new
RasterWindowContext_android((void*)&platformData, fDisplayParams);
On 2016/06/07 16:05:30, djsollen wrote:
> this should use the same factory creation as the other backends and look like
> RasterWindowContext::Create(...)

I'm deleting RasterWindowContext. I don't see any benefit of the factory
creation in Raster use case since there isn't much common code across different
platforms...

[djsollen, 2016-06-07 19:31:48]

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/RasterWindowContext_android.h (right):

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.h:23: bool isValid()
override { return true; }
should only return true if fNativeWindow != nullptr

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.h:24: void
resize(uint32_t w, uint32_t h) override {
implement this because I may want to try to use the code in the framework which
requires a resize.

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.h:32: GrBackendContext
getBackendContext() override { return (GrBackendContext) nullptr; }
put in common RasterWindowContext.h

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.h:34: protected:
put in common RasterWindowContext.h

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:68: fWindowContext = new
RasterWindowContext_android((void*)&platformData, fDisplayParams);
use RasterWindowContext::Create for consistency.

[liyuqian, 2016-06-07 19:53:02]

RasterWindowContext is back.

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/RasterWindowContext_android.h (right):

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.h:23: bool isValid()
override { return true; }
On 2016/06/07 19:31:48, djsollen wrote:
> should only return true if fNativeWindow != nullptr

Done.

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.h:24: void
resize(uint32_t w, uint32_t h) override {
On 2016/06/07 19:31:48, djsollen wrote:
> implement this because I may want to try to use the code in the framework
which
> requires a resize.

In the next CL.

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.h:32: GrBackendContext
getBackendContext() override { return (GrBackendContext) nullptr; }
On 2016/06/07 19:31:48, djsollen wrote:
> put in common RasterWindowContext.h

Done.

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/RasterWindowContext_android.h:34: protected:
On 2016/06/07 19:31:48, djsollen wrote:
> put in common RasterWindowContext.h

Done.

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2041193004/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:68: fWindowContext = new
RasterWindowContext_android((void*)&platformData, fDisplayParams);
On 2016/06/07 19:31:48, djsollen wrote:
> use RasterWindowContext::Create for consistency.

Done.

[djsollen, 2016-06-07 20:09:07]

lgtm

[liyuqian, 2016-06-07 20:16:03]

The CQ bit was checked by liyuqian@google.com

[commit-bot: I haz the power, 2016-06-07 20:31:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041193004/140001

[commit-bot: I haz the power, 2016-06-07 21:22:41]

Description was changed from

==========
Implement Raster Backend on Android Viewer App

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041193004
==========

to

==========
Implement Raster Backend on Android Viewer App

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041193004

Committed:
https://skia.googlesource.com/skia/+/d94ad5823b7da7f115e997d12828314e290981f6
==========

[commit-bot: I haz the power, 2016-06-07 21:22:43]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/d94ad5823b7da7f115e997d12828314e290981f6