Hello cprince,
I'd like you to do a code review. Please execute
g4 diff -c 10408353
or point your web browser to
http://mondrian/10408353
to review the following code:
Change 10408353 by nigel...@nigeltao-srcgears2 on 2009/03/09 11:33:41 *pending*
Implement canvas' {create,get,put}ImageData.
PRESUBMIT=passed
R=cprince
[email protected]
DELTA=277 (230 added, 2 deleted, 45 changed)
OCL=10408353
Affected files ...
... //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#24 edit
... //depot/googleclient/gears/opensource/gears/canvas/canvas.h#19 edit
...
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc#20
edit
...
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h#15
edit
... //depot/googleclient/gears/opensource/gears/test/manual/canvas.html#3 edit
277 delta lines: 230 added, 2 deleted, 45 changed
Also consider running:
g4 lint -c 10408353
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [email protected].
Change 10408353 by nigel...@nigeltao-srcgears2 on 2009/03/09 11:33:41 *pending*
Implement canvas' {create,get,put}ImageData.
Affected files ...
... //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#24 edit
... //depot/googleclient/gears/opensource/gears/canvas/canvas.h#19 edit
...
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc#20
edit
...
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h#15
edit
... //depot/googleclient/gears/opensource/gears/test/manual/canvas.html#3 edit
==== //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#24 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/canvas/canvas.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas.cc 2009-03-09
10:35:51.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas.cc 2009-03-08
18:10:59.000000000 +1100
@@ -249,12 +249,14 @@
new_bitmap.setConfig(skia_config, new_width, new_height);
new_bitmap.allocPixels();
- if (width() != 0 && height() != 0) {
+ int old_width = GetWidth();
+ int old_height = GetHeight();
+ if (old_width != 0 && old_height != 0) {
SkCanvas new_canvas(new_bitmap);
SkScalar x_scale = SkDoubleToScalar(
- static_cast<double>(new_width) / width());
+ static_cast<double>(new_width) / old_width);
SkScalar y_scale = SkDoubleToScalar(
- static_cast<double>(new_height) / height());
+ static_cast<double>(new_height) / old_height);
if (!new_canvas.scale(x_scale, y_scale)) {
context->SetException(STRING16(L"Could not resize the image."));
return;
@@ -267,12 +269,12 @@
}
void GearsCanvas::GetWidth(JsCallContext *context) {
- int its_width = width();
+ int its_width = GetWidth();
context->SetReturnValue(JSPARAM_INT, &its_width);
}
void GearsCanvas::GetHeight(JsCallContext *context) {
- int its_height = height();
+ int its_height = GetHeight();
context->SetReturnValue(JSPARAM_INT, &its_height);
}
@@ -284,7 +286,7 @@
context->GetArguments(ARRAYSIZE(args), args);
if (context->is_exception_set())
return;
- ResetCanvas(new_width, height());
+ ResetCanvas(new_width, GetHeight());
}
void GearsCanvas::SetHeight(JsCallContext *context) {
@@ -295,7 +297,7 @@
context->GetArguments(ARRAYSIZE(args), args);
if (context->is_exception_set())
return;
- ResetCanvas(width(), new_height);
+ ResetCanvas(GetWidth(), new_height);
}
void GearsCanvas::GetContext(JsCallContext *context) {
@@ -326,12 +328,17 @@
context->SetReturnValue(JSPARAM_MODULE, rendering_context_);
}
-int GearsCanvas::width() const {
+int GearsCanvas::GetWidth() const {
return skia_bitmap_->width();
}
-int GearsCanvas::height() const {
+int GearsCanvas::GetHeight() const {
return skia_bitmap_->height();
+}
+
+SkBitmap *GearsCanvas::GetSkBitmap() {
+ EnsureBitmapPixelsAreAllocated();
+ return skia_bitmap_.get();
}
double GearsCanvas::alpha() const {
@@ -371,7 +378,7 @@
bool GearsCanvas::IsRectValid(const SkIRect &rect) {
return rect.fLeft <= rect.fRight && rect.fTop <= rect.fBottom &&
rect.fLeft >= 0 && rect.fTop >= 0 &&
- rect.fRight <= width() && rect.fBottom <= height();
+ rect.fRight <= GetWidth() && rect.fBottom <= GetHeight();
}
// Skia's SkPorterDuff values are all non-negative.
==== //depot/googleclient/gears/opensource/gears/canvas/canvas.h#19 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/canvas/canvas.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas.h 2009-03-09
10:35:51.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas.h 2009-03-08
18:11:02.000000000 +1100
@@ -108,13 +108,15 @@
// fail silently without returning an error, as per the HTML5 canvas spec.
// But if given an argument that HTML5 canvas supports but we don't,
// the setters (set_composite_operation, to be precise) returns false.
- int width() const;
- int height() const;
double alpha() const;
void set_alpha(double new_alpha);
std::string16 composite_operation() const;
// Returns false if given a mode HTML5 canvas supports but we don't.
bool set_composite_operation(std::string16 new_composite_op);
+
+ int GetWidth() const;
+ int GetHeight() const;
+ SkBitmap *GetSkBitmap();
private:
// Resets the Canvas to the specified dimensions and fills it with
transparent
====
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc#20
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc
2009-03-06 14:34:56.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.cc
2009-03-10 14:38:10.000000000 +1100
@@ -23,9 +23,13 @@
// OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
// ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#include <algorithm>
+#include <cmath>
+
#include "gears/canvas/canvas_rendering_context_2d.h"
#include "gears/base/common/js_runner.h"
+#include "third_party/skia/include/core/SkColorPriv.h"
#include "third_party/skia/include/core/SkPorterDuff.h"
#include "third_party/skia/include/utils/SkParse.h"
@@ -40,8 +44,11 @@
// gracefully. I think that WebKit's Skia-backed canvas already does this.
DECLARE_DISPATCHER(GearsCanvasRenderingContext2D);
+
const std::string
GearsCanvasRenderingContext2D::kModuleName("GearsCanvasRenderingContext2D");
+
+const int GearsCanvasRenderingContext2D::kMaxImageDataSize(1024);
using canvas::skia_config;
@@ -666,46 +673,200 @@
// TODO(nigeltao): Are the colors premultiplied? If so, unpremultiply them.
}
+static U8CPU InvScaleByte(U8CPU component, uint32_t scale)
+{
+ assert(component == static_cast<uint8_t>(component));
+ return (component * scale + 0x8000) >> 16;
+}
+
+// TODO(nigeltao): Submit Unpremultiply (and InvScaleByte) into upstream Skia.
+static SkColor Unpremultiply(SkPMColor pm) {
+ if (pm == 0) {
+ return 0;
+ }
+ uint32_t a = SkGetPackedA32(pm);
+ uint32_t scale = (255 << 16) / a;
+ return SkColorSetARGB(a,
+ InvScaleByte(SkGetPackedR32(pm), scale),
+ InvScaleByte(SkGetPackedG32(pm), scale),
+ InvScaleByte(SkGetPackedB32(pm), scale));
+}
+
void GearsCanvasRenderingContext2D::CreateImageData(JsCallContext *context) {
- int width, height;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_INT, &width },
- { JSPARAM_REQUIRED, JSPARAM_INT, &height }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
+ double sw, sh;
+ JsArgument args[] = {
+ { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &sw },
+ { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &sh }
+ };
+ context->GetArguments(ARRAYSIZE(args), args);
+ if (context->is_exception_set())
+ return;
+
+ int w = static_cast<int>(fabs(sw));
+ int h = static_cast<int>(fabs(sh));
+ if (w <= 0 || h <= 0) {
+ context->SetException(STRING16(L"INDEX_SIZE_ERR"));
+ return;
+ }
+ if (w > kMaxImageDataSize || h > kMaxImageDataSize) {
+ context->SetException(
+ STRING16(L"CreateImageData called for too large an image slice."));
+ return;
+ }
+
+ scoped_ptr<JsArray> data(module_environment_->js_runner_->NewArray());
+ for (int i = (w * h * 4) - 1; i >= 0; i--) {
+ data->SetElementInt(i, 0);
+ }
+
+ scoped_ptr<JsObject> result(module_environment_->js_runner_->NewObject());
+ result->SetPropertyInt(STRING16(L"width"), w);
+ result->SetPropertyInt(STRING16(L"height"), h);
+ result->SetPropertyArray(STRING16(L"data"), data.get());
+ context->SetReturnValue(JSPARAM_OBJECT, result.get());
}
void GearsCanvasRenderingContext2D::GetImageData(JsCallContext *context) {
- int sx, sy, sw, sh;
- JsArgument args[] = {
- { JSPARAM_REQUIRED, JSPARAM_INT, &sx },
- { JSPARAM_REQUIRED, JSPARAM_INT, &sy },
- { JSPARAM_REQUIRED, JSPARAM_INT, &sw },
- { JSPARAM_REQUIRED, JSPARAM_INT, &sh }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
+ double sx, sy, sw, sh;
+ JsArgument args[] = {
+ { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &sx },
+ { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &sy },
+ { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &sw },
+ { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &sh }
+ };
+ context->GetArguments(ARRAYSIZE(args), args);
+ if (context->is_exception_set())
+ return;
+
+ int x = static_cast<int>(std::min(sx, sx + sw));
+ int y = static_cast<int>(std::min(sy, sy + sh));
+ int w = static_cast<int>(std::max(sx, sx + sw)) - x;
+ int h = static_cast<int>(std::max(sy, sy + sh)) - y;
+ if (w <= 0 || h <= 0) {
+ context->SetException(STRING16(L"INDEX_SIZE_ERR"));
+ return;
+ }
+ if (w > kMaxImageDataSize || h > kMaxImageDataSize) {
+ context->SetException(
+ STRING16(L"GetImageData called for too large an image slice."));
+ return;
+ }
+ SkBitmap *bitmap = gears_canvas_->GetSkBitmap();
+ assert(bitmap->config() == SkBitmap::kARGB_8888_Config);
+ SkAutoLockPixels bitmap_lock(*bitmap);
+ if (x < 0 || y < 0 ||
+ x + w > bitmap->width() ||
+ y + h > bitmap->height()) {
+ // The HTML5 spec says to extend with transparent black, outside the
canvas,
+ // but for now we'll match Mozilla and throw an exception. In the future,
+ // we could change behavior to match the spec, but we will have to be
+ // careful with the raw pointer arithmetic below.
+ context->SetException(
+ STRING16(L"GetImageData called for an out-of-bounds image slice."));
+ return;
+ }
+
+ scoped_ptr<JsArray> data(module_environment_->js_runner_->NewArray());
+ // Provide a hint to the JS engine as to the eventual length of this array.
+ data->SetElementUndefined(w * h * 4 - 1);
+
+ int data_index = 0;
+ for (int j = 0; j < h; j++) {
+ uint32_t* source = bitmap->getAddr32(x, y + j);
+ for (int i = 0; i < w; i++) {
+ SkColor color = Unpremultiply(source[i]);
+ data->SetElementInt(data_index++, SkColorGetR(color));
+ data->SetElementInt(data_index++, SkColorGetG(color));
+ data->SetElementInt(data_index++, SkColorGetB(color));
+ data->SetElementInt(data_index++, SkColorGetA(color));
+ }
+ }
+
+ scoped_ptr<JsObject> result(module_environment_->js_runner_->NewObject());
+ result->SetPropertyInt(STRING16(L"width"), w);
+ result->SetPropertyInt(STRING16(L"height"), h);
+ result->SetPropertyArray(STRING16(L"data"), data.get());
+ context->SetReturnValue(JSPARAM_OBJECT, result.get());
}
void GearsCanvasRenderingContext2D::PutImageData(JsCallContext *context) {
scoped_ptr<JsObject> image_data;
- int dx, dy, dirty_x, dirty_y, dirty_width, dirty_height;
+ double dx, dy, dirty_x, dirty_y, dirty_width, dirty_height;
JsArgument args[] = {
{ JSPARAM_REQUIRED, JSPARAM_OBJECT, as_out_parameter(image_data) },
- { JSPARAM_REQUIRED, JSPARAM_INT, &dx },
- { JSPARAM_REQUIRED, JSPARAM_INT, &dy },
- { JSPARAM_OPTIONAL, JSPARAM_INT, &dirty_x },
- { JSPARAM_OPTIONAL, JSPARAM_INT, &dirty_y },
- { JSPARAM_OPTIONAL, JSPARAM_INT, &dirty_width },
- { JSPARAM_OPTIONAL, JSPARAM_INT, &dirty_height }
- };
- context->GetArguments(ARRAYSIZE(args), args);
- if (context->is_exception_set())
- return;
- context->SetException(STRING16(L"Unimplemented"));
-}
+ { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &dx },
+ { JSPARAM_REQUIRED, JSPARAM_DOUBLE, &dy },
+ { JSPARAM_OPTIONAL, JSPARAM_DOUBLE, &dirty_x },
+ { JSPARAM_OPTIONAL, JSPARAM_DOUBLE, &dirty_y },
+ { JSPARAM_OPTIONAL, JSPARAM_DOUBLE, &dirty_width },
+ { JSPARAM_OPTIONAL, JSPARAM_DOUBLE, &dirty_height }
+ };
+ context->GetArguments(ARRAYSIZE(args), args);
+ if (context->is_exception_set())
+ return;
+
+ int x = static_cast<int>(dx);
+ int y = static_cast<int>(dy);
+ int w, h;
+ scoped_ptr<JsArray> a;
+ int a_length;
+ if (!image_data->GetPropertyAsInt(STRING16(L"width"), &w) ||
+ !image_data->GetPropertyAsInt(STRING16(L"height"), &h) ||
+ !image_data->GetPropertyAsArray(STRING16(L"data"), as_out_parameter(a))
||
+ !a->GetLength(&a_length)||
+ w * h * 4 != a_length) {
+ context->SetException(
+ STRING16(L"PutImageData dimensions do not match."));
+ return;
+ }
+ if (w <= 0 || h <= 0) {
+ context->SetException(STRING16(L"INDEX_SIZE_ERR"));
+ return;
+ }
+ if (w > kMaxImageDataSize || h > kMaxImageDataSize) {
+ context->SetException(
+ STRING16(L"PutImageData called for too large an image slice."));
+ return;
+ }
+ SkBitmap *bitmap = gears_canvas_->GetSkBitmap();
+ assert(bitmap->config() == SkBitmap::kARGB_8888_Config);
+ SkAutoLockPixels bitmap_lock(*bitmap);
+ if (x < 0 || y < 0 ||
+ x + w > bitmap->width() ||
+ y + h > bitmap->height()) {
+ // The HTML5 spec says to silently ignore put-pixel calls outside the
+ // canvas, but for now we'll throw an exception, like our GetImageData.
+ context->SetException(
+ STRING16(L"PutImageData called for an out-of-bounds image slice."));
+ return;
+ }
+
+ // TODO(nigeltao): Handle dirty_x, dirty_y, dirty_width, dirty_height as
+ // per the HTML5 spec.
+
+ int data_index = 0;
+ for (int j = 0; j < h; j++) {
+ uint32_t* source = bitmap->getAddr32(x, y + j);
+ for (int i = 0; i < w; i++) {
+ int pixel_r = 0;
+ int pixel_g = 0;
+ int pixel_b = 0;
+ int pixel_a = 0;
+ // The HTML5 spec says that if GetElementAsInt fails (e.g. if the
+ // element's value is undefined), treat is as zero.
+ a->GetElementAsInt(data_index++, &pixel_r);
+ a->GetElementAsInt(data_index++, &pixel_g);
+ a->GetElementAsInt(data_index++, &pixel_b);
+ a->GetElementAsInt(data_index++, &pixel_a);
+ pixel_r = std::max(0, std::min(255, pixel_r));
+ pixel_g = std::max(0, std::min(255, pixel_g));
+ pixel_b = std::max(0, std::min(255, pixel_b));
+ pixel_a = std::max(0, std::min(255, pixel_a));
+ source[i] = SkPreMultiplyARGB(
+ static_cast<U8CPU>(pixel_a),
+ static_cast<U8CPU>(pixel_r),
+ static_cast<U8CPU>(pixel_g),
+ static_cast<U8CPU>(pixel_b));
+ }
+ }
+}
====
//depot/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h#15
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h
2009-03-09 10:35:52.000000000 +1100
+++ googleclient/gears/opensource/gears/canvas/canvas_rendering_context_2d.h
2009-03-08 19:10:04.000000000 +1100
@@ -46,6 +46,10 @@
public:
static const std::string kModuleName;
+ // {Create,Get,Put}ImageData won't return or accept any ImageData that is
+ // wider than kMaxImageDataSize, or taller than kMaxImageDataSize.
+ static const int kMaxImageDataSize;
+
GearsCanvasRenderingContext2D();
virtual ~GearsCanvasRenderingContext2D();
==== //depot/googleclient/gears/opensource/gears/test/manual/canvas.html#3 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/test/manual/canvas.html
====
# action=edit type=text
--- googleclient/gears/opensource/gears/test/manual/canvas.html 2009-03-09
10:43:47.000000000 +1100
+++ googleclient/gears/opensource/gears/test/manual/canvas.html 2009-03-09
10:47:30.000000000 +1100
@@ -6,6 +6,9 @@
<p>Browser's native canvas:</p>
<canvas id="bnc" width="100px" height="80px" style="border:1px solid black">
</canvas>
+<hr>
+<p>Difference:</p>
+<img id="dc" width="100px" height="80px" style="border:1px solid black">
<script type="text/javascript" src="../../sdk/gears_init.js"></script>
<script type="text/javascript">
@@ -39,6 +42,9 @@
}
+var store = null;
+
+
var gearsCanvas = null;
try {
gearsCanvas = google.gears.factory.create('beta.canvas');
@@ -54,8 +60,8 @@
var localServer = google.gears.factory.create('beta.localserver');
localServer.removeStore("test_manual_canvas");
- var store = localServer.createStore("test_manual_canvas");
- var capturedUrl = parseInt(Math.random() * 1000) + '.png';
+ store = localServer.createStore("test_manual_canvas");
+ var capturedUrl = 'g' + parseInt(Math.random() * 1000) + '.png';
store.captureBlob(gearsCanvas.toBlob(), capturedUrl, 'image/png');
document.getElementById('gc').src = capturedUrl;
}
@@ -73,6 +79,54 @@
}
+var diffCanvas = null;
+try {
+ diffCanvas = google.gears.factory.create('beta.canvas');
+} catch (e) {
+ // Silently ignore any exceptions for those Gears builds that don't have a
+ // canvas implementation.
+}
+if (gearsCanvas && nativeContext && diffCanvas) {
+ diffCanvas.width = 100;
+ diffCanvas.height = 80;
+
+ var gearsContext = gearsCanvas.getContext('gears-2d');
+ var diffContext = diffCanvas.getContext('gears-2d');
+
+ var gearsImageData = gearsContext.getImageData(0, 0, 100, 80);
+ var nativeImageData = nativeContext.getImageData(0, 0, 100, 80);
+ var diffImageData = diffContext.createImageData(100, 80);
+
+ var w = diffImageData.width;
+ var h = diffImageData.height;
+ for (var i = 0; i < w * h; i++) {
+ var d = 0;
+ var base = 4 * i;
+ for (var j = 0; j < 3; j++) {
+ d += Math.abs(
+ gearsImageData.data[base + j] - nativeImageData.data[base + j]);
+ }
+ if (d > 0) {
+ // Exaggerate the difference, to make it easier to see.
+ d = 32 + d;
+ }
+ if (d > 255) {
+ d = 255;
+ }
+ diffImageData.data[base + 0] = d;
+ diffImageData.data[base + 1] = d;
+ diffImageData.data[base + 2] = d;
+ diffImageData.data[base + 3] = 255;
+ }
+
+ diffContext.putImageData(diffImageData, 0, 0);
+
+ var capturedUrl = 'd' + parseInt(Math.random() * 1000) + '.png';
+ store.captureBlob(diffCanvas.toBlob(), capturedUrl, 'image/png');
+ document.getElementById('dc').src = capturedUrl;
+}
+
+
</script>
</body>
</html>
