Smallest possible desktop application that uses Skia to render stuff.

experimental/SkiaExamples/BaseExample.cpp

+/*
+ * Copyright 2013 Google Inc.
+ *
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ *
+ */
+
+#include "BaseExample.h"
+
+#include "gl/GrGLInterface.h"
+#include "SkGpuDevice.h"
+
+BaseExample::BaseExample(void* hWnd, int argc, char** argv)
+    : INHERITED(hWnd)
+{
+    this->setConfig(SkBitmap::kARGB_8888_Config);
+    this->setVisibleP(true);
+    this->setClipToBounds(false);
+
+    // Software rendering by default
+    fType = kRaster_DeviceType;

[caryclark, 2013/06/10 15:48:40]

setupBackend also sets fType -- pick one or the other

[sglez, 2013/06/11 04:24:38]

Removed this and also the call to setupBackend below, to avoid calling it twice
by subclasses calling this constructor.

+
+    setupBackend(fType);
+}
+
+BaseExample::~BaseExample() {
+    if (fType == kGPU_DeviceType) {

[caryclark, 2013/06/10 15:48:40]

This style is OK and we (Skia) are not very consistent about it, but

if (kGPU_DeviceType == fType) {

allows detecting this error consistently:

if (kGPU_DeviceType = fType) { // flags an assignment error on all compilers
if (fType = kGPU_DeviceType) { // may escape notice on some compilers

[sglez, 2013/06/11 04:24:38]

Actually I have been trying to keep to that style, force of habit

On 2013/06/10 15:48:40, caryclark wrote:

+        SkSafeUnref(fContext);
+        SkSafeUnref(fInterface);
+        SkSafeUnref(fRenderTarget);
+    }
+}
+
+void BaseExample::setupBackend(DeviceType type) {
+    fType = type;
+    if(type == kGPU_DeviceType) {

[caryclark, 2013/06/10 15:48:40]

for long blocks like this, I'd rather see

if (kGPU_DeviceType != type) {
    return;
}
AttachmentInfo attachmentInfo;
...

to minimize the size of large indentations

+        AttachmentInfo attachmentInfo;
+        bool result = attach(kNativeGL_BackEndType, 0 /*msaa*/, &attachmentInfo);
+
+        if (false == result) {
+            SkDebugf("Not possible to create GL backend. Defaulting to raster.");
+            fType = kRaster_DeviceType;
+            detach();
+            return;

[caryclark, 2013/06/10 15:48:40]

does the caller need to know that this failed? (e.g., should setupBackend return
bool to indicate success?)

[sglez, 2013/06/11 04:24:38]

The caller would probably like to know this. 
Changed to return a bool indicating success or failure 

+        }
+
+        const GrGLInterface* interface = GrGLCreateNativeInterface();
+        fInterface = const_cast<GrGLInterface*>(interface);

[caryclark, 2013/06/10 15:48:40]

Throwing away a const is a red flag. Either the interface is broken or this
usage is. At minimum, this requires a comment to show why you think casting is
OK.

[sglez, 2013/06/11 04:24:38]

Sorry about the const_cast, fixed by setting the class member to const.

+        SkASSERT(NULL != interface);
+
+        fContext = GrContext::Create(kOpenGL_GrBackend, (GrBackendContext)interface);
+        SkASSERT(NULL != fContext);
+
+        GrBackendRenderTargetDesc desc;
+        desc.fWidth = SkScalarRound(width());
+        desc.fHeight = SkScalarRound(height());
+        desc.fConfig = kSkia8888_GrPixelConfig;
+        desc.fOrigin = kBottomLeft_GrSurfaceOrigin;
+        desc.fSampleCnt = attachmentInfo.fSampleCount;
+        desc.fStencilBits = attachmentInfo.fStencilBits;
+
+        GrGLint buffer;
+        interface->fGetIntegerv(GL_FRAMEBUFFER_BINDING, &buffer);
+        desc.fRenderTargetHandle = buffer;
+
+        fRenderTarget = fContext->wrapBackendRenderTarget(desc);
+
+        fContext->setRenderTarget(fRenderTarget);
+    }
+}
+
+SkCanvas* BaseExample::createCanvas() {
+    if (fType == kGPU_DeviceType &&
+        NULL != fContext && NULL != fRenderTarget) {

[caryclark, 2013/06/10 15:48:40]

indent by 4 more so it doesn't look like the body of the if

[caryclark, 2013/06/10 15:48:40]

When can fContext be NULL?

If fRenderTarget returned NULL above, should the fType have been set to
kRaster_DeviceType?

[sglez, 2013/06/11 04:24:38]

Done.

[sglez, 2013/06/11 04:24:38]

The fContext is just a precaution. 
You are right. fType is now changed here.

+        SkAutoTUnref<SkDevice> device(new SkGpuDevice(fContext, fRenderTarget));
+        return new SkCanvas(device);
+    } else {
+        return INHERITED::createCanvas();
+    }
+}
+
+void BaseExample::draw(SkCanvas* canvas) {
+    if (getDeviceType() == kGPU_DeviceType) {
+        present();

[caryclark, 2013/06/10 15:48:40]

When draw() is overridden by a child, should present() be called before or after
the other drawing?

[sglez, 2013/06/11 04:24:38]

 present() should be called at the end. It is a buffer swap when using GL.

+    }
+}
+