Create a standalone example for using Skia with SDL

Create a standalone example for using Skia with SDL

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/04d52f3a92bb3d72e035a7c002926953edb0b505

[joshualitt, 2015-11-12 21:49:21]

joshualitt@google.com changed reviewers:
+ joshualitt@google.com, robertphillips@google.com

[joshualitt, 2015-11-12 21:49:22]

create an example mash up of SDL and Skia

[robertphillips, 2015-11-13 14:02:52]

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample.cpp
File example/SkiaSDLExample.cpp (right):

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:25: 
// This application is a simple example of how to combine SDL and Skia
// It demonstrates:
//   how to setup gpu rendering to the main window
//   how to perform cpu-side rendering and draw the result to the gpu-backed
screen
//   draw simple primitives (rectangles)

?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:26: struct ApplicationState {
// Storage for the user created rectangles. The last one may still be being
edited.

?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:42: if (event.motion.state == SDL_PRESSED) {
It might be clearer to make this "SkRect& rect ..." and then just update the
bottom & right slots

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:71: 
// Create a pentagram
?

maybe rename the function ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:72: static SkPath setup_concave_path() {
static const int kNumPoints = 5;

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:80: concavePath.moveTo(points[0]);
shouldn't this loop start at 1 ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:83: }
concavePath.close() ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:101: #if defined(SK_BUILD_FOR_ANDROID)
Can we pull out the two shared lines ?

// For Android we need to set up for OpenGL ES and we make the window hi res &
full screen

?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:110:
SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, 0);
// For all other clients we use the core profile and operate in a window

?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:114: #endif
Why not 8 ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:118: SDL_GL_SetAttribute(SDL_GL_DOUBLEBUFFER, 1);
Can we set depth to 0 ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:119: SDL_GL_SetAttribute(SDL_GL_DEPTH_SIZE, 24);
// Skia needs 8 stencil bits

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:124: 
put this in the SK_SUPPORT_GPU block ?

where is msaaSampleCount coming from ?
should it be "static const int kMSAASampleCount = 0;" ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:144: 
rm this ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:146: 
Add "SDL_Window*" here and remove the above declaration ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:178: 
uncomment this -> comment this out ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:187: 
// Wrap the frame buffer object attached to the screen in a Skia render target
so Skia can render to it

?

Should this be a subroutine - wrap_fbo_for_skia ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:193: desc.fOrigin = kBottomLeft_GrSurfaceOrigin;
kMSAASampleCount ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:217: 
Can this be  "SkImage* make_a_raster_image()"  ?

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:218: // create a surface for CPU rasterization
offscreenInfo seem unused

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:220: offscreenInfo.makeWH(2000, 2000);
That's a big hunk o' memory
Can it just be the size of the concave path's bounding box ?

[joshualitt, 2015-11-13 18:51:56]

feedback incorporated

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample.cpp
File example/SkiaSDLExample.cpp (right):

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:25: 
On 2015/11/13 14:02:52, robertphillips wrote:
> // This application is a simple example of how to combine SDL and Skia
> // It demonstrates:
> //   how to setup gpu rendering to the main window
> //   how to perform cpu-side rendering and draw the result to the gpu-backed
> screen
> //   draw simple primitives (rectangles)
> 
> ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:26: struct ApplicationState {
On 2015/11/13 14:02:51, robertphillips wrote:
> // Storage for the user created rectangles. The last one may still be being
> edited.
> 
> ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:42: if (event.motion.state == SDL_PRESSED) {
On 2015/11/13 14:02:51, robertphillips wrote:
> It might be clearer to make this "SkRect& rect ..." and then just update the
> bottom & right slots

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:71: 
On 2015/11/13 14:02:51, robertphillips wrote:
> // Create a pentagram
> ?
> 
> maybe rename the function ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:72: static SkPath setup_concave_path() {
On 2015/11/13 14:02:52, robertphillips wrote:
> static const int kNumPoints = 5;

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:80: concavePath.moveTo(points[0]);
On 2015/11/13 14:02:51, robertphillips wrote:
> shouldn't this loop start at 1 ?

dunno, it looks right.  Why rock the boat?  (Unless you have a strong opinion)

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:83: }
On 2015/11/13 14:02:51, robertphillips wrote:
> concavePath.close() ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:101: #if defined(SK_BUILD_FOR_ANDROID)
On 2015/11/13 14:02:51, robertphillips wrote:
> Can we pull out the two shared lines ?
> 
> // For Android we need to set up for OpenGL ES and we make the window hi res &
> full screen
> 
> ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:110:
SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, 0);
On 2015/11/13 14:02:52, robertphillips wrote:
> // For all other clients we use the core profile and operate in a window
> 
> ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:114: #endif
On 2015/11/13 14:02:51, robertphillips wrote:
> Why not 8 ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:118: SDL_GL_SetAttribute(SDL_GL_DOUBLEBUFFER, 1);
On 2015/11/13 14:02:52, robertphillips wrote:
> Can we set depth to 0 ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:119: SDL_GL_SetAttribute(SDL_GL_DEPTH_SIZE, 24);
On 2015/11/13 14:02:51, robertphillips wrote:
> // Skia needs 8 stencil bits

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:124: 
I setup the static variable, but got rid of the SK_SUPPORT_GPU.  I'm not going
to bother trying to get this to work without GPU support

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:144: 
On 2015/11/13 14:02:51, robertphillips wrote:
> rm this ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:146: 
On 2015/11/13 14:02:51, robertphillips wrote:
> Add "SDL_Window*" here and remove the above declaration ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:178: 
On 2015/11/13 14:02:51, robertphillips wrote:
> uncomment this -> comment this out ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:187: 
On 2015/11/13 14:02:51, robertphillips wrote:
> // Wrap the frame buffer object attached to the screen in a Skia render target
> so Skia can render to it
> 
> ?
> 
> Should this be a subroutine - wrap_fbo_for_skia ?

Then its just a function which takes 10 arguments.  If this was production code
I would, but as an example its kind of nice to have all of the code in one
place.  If you have a strong preference I will change it.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:193: desc.fOrigin = kBottomLeft_GrSurfaceOrigin;
On 2015/11/13 14:02:52, robertphillips wrote:
> kMSAASampleCount ?

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:217: 
On 2015/11/13 14:02:52, robertphillips wrote:
> Can this be  "SkImage* make_a_raster_image()"  ?

see my response to wrap_fbo_for_skia.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:218: // create a surface for CPU rasterization
On 2015/11/13 14:02:52, robertphillips wrote:
> offscreenInfo seem unused

Acknowledged.

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:220: offscreenInfo.makeWH(2000, 2000);
On 2015/11/13 14:02:51, robertphillips wrote:
> That's a big hunk o' memory
> Can it just be the size of the concave path's bounding box ?

Acknowledged.

[robertphillips, 2015-11-13 19:02:59]

lgtm

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample.cpp
File example/SkiaSDLExample.cpp (right):

https://codereview.chromium.org/1442573003/diff/20001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:80: concavePath.moveTo(points[0]);
On 2015/11/13 18:51:55, joshualitt wrote:
> On 2015/11/13 14:02:51, robertphillips wrote:
> > shouldn't this loop start at 1 ?
> 
> dunno, it looks right.  Why rock the boat?  (Unless you have a strong opinion)

Don't we do a moveTo followed by a lineTo to the same point?

https://codereview.chromium.org/1442573003/diff/40001/example/SkiaSDLExample.cpp
File example/SkiaSDLExample.cpp (right):

https://codereview.chromium.org/1442573003/diff/40001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:202: desc.fOrigin = kBottomLeft_GrSurfaceOrigin;
Is the comment still right ?

https://codereview.chromium.org/1442573003/diff/40001/example/SkiaSDLExample....
example/SkiaSDLExample.cpp:220: ApplicationState state;
I'm surprised SkTArray doesn't gripe about being bzeroed

[joshualitt, 2015-11-13 19:11:01]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-11-13 19:11:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1442573003/#ps60001
(title: "feedback inc")

[commit-bot: I haz the power, 2015-11-13 19:11:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442573003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442573003/60001

[commit-bot: I haz the power, 2015-11-13 19:22:23]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/04d52f3a92bb3d72e035a7c002926953edb0b505