Exposing a pointer to the raw data.

Exposing a pointer to the raw data.

[reed1, 2011-06-13 12:22:49]

I am not in favor of exposing an API that reveals the internal structure, thus
forcing a specific one.

I think it would be great to add efficient getters to return as column-major
floats the existing matrix.

e.g.

void asColumnMajorFloats(float[]) const;

(or some other, shorter, name).

If this sort of API proves to be a performance issue, we can always revisit this
CL.

[reed1, 2011-06-13 12:23:44]

That said, I'm completely fine with removing the definition of MSCALAR_IS_DOUBLE
in the header. That decision should be left to the Makefile or its equivalent
(as we do already for SK_SCALAR_IS_FLOAT).

[vollick, 2011-06-13 13:55:04]

I'm trying to use SkMatrix44 in Chromium. A reviewer of my CL brought up
performance issues when copying all the elements from the Skia matrix into an
array for OpenGL. He recommended passing GL a pointer to the data, which was the
impetus for this Skia CL.

I understand your hesitancy to commit to implementation details like using
floats and column major matrices. I could include #ifdef's to allow users to
switch between row and column major. Templating SkMatrix44 is another option --
something like SkMatrix44<typename T, bool RowMajor> (possibly with other
template arguments). This would allow the user specify the storage, and would
allow more than one flavor of SkMatrix44 to be used in the same compilation
unit. I don't think that there's another storage scheme other than a dense array
would be sensible for a 4x4 matrix, so in terms of storage it seems like the
only decision the user needs to make is whether or not the matrix should be row
major -- please let me know if I've got that wrong.

If we can let the user customize the storage of the matrix, would you be more
open to exposing a raw pointer to the data?

[reed1, 2011-06-13 13:59:48]

I guess, before we entertain any of that, I'd like to see a profile
where copying the 16 values is a performance hit. We copy the skia
matrix today into GL for its gl-backend, and have seen hit for that
copy.

I think a compile option to change row -vs- col major would make using
the API difficult, given the existing accessors.

On Mon, Jun 13, 2011 at 9:55 AM,  <vollick@chromium.org> wrote:
> I'm trying to use SkMatrix44 in Chromium. A reviewer of my CL brought up
> performance issues when copying all the elements from the Skia matrix
> into an array for OpenGL. He recommended passing GL a pointer to the
> data, which was the impetus for this Skia CL.
>
> I understand your hesitancy to commit to implementation details like
> using floats and column major matrices. I could include #ifdef's to
> allow users to switch between row and column major. Templating
> SkMatrix44 is another option -- something like SkMatrix44<typename T,
> bool RowMajor> (possibly with other template arguments). This would
> allow the user specify the storage, and would allow more than one flavor
> of SkMatrix44 to be used in the same compilation unit. I don't think
> that there's another storage scheme other than a dense array would be
> sensible for a 4x4 matrix, so in terms of storage it seems like the only
> decision the user needs to make is whether or not the matrix should be
> row major -- please let me know if I've got that wrong.
>
> If we can let the user customize the storage of the matrix, would you be
> more open to exposing a raw pointer to the data?
>
> http://codereview.appspot.com/4579050/
>

[reed1, 2011-06-13 14:25:52]

Oops, typo -- we *haven't* seen any perf hit for the copy. Sorry.

On Mon, Jun 13, 2011 at 9:59 AM, Mike Reed <reed@google.com> wrote:
> I guess, before we entertain any of that, I'd like to see a profile
> where copying the 16 values is a performance hit. We copy the skia
> matrix today into GL for its gl-backend, and have seen hit for that
> copy.
>
> I think a compile option to change row -vs- col major would make using
> the API difficult, given the existing accessors.
>
> On Mon, Jun 13, 2011 at 9:55 AM,  <vollick@chromium.org> wrote:
>> I'm trying to use SkMatrix44 in Chromium. A reviewer of my CL brought up
>> performance issues when copying all the elements from the Skia matrix
>> into an array for OpenGL. He recommended passing GL a pointer to the
>> data, which was the impetus for this Skia CL.
>>
>> I understand your hesitancy to commit to implementation details like
>> using floats and column major matrices. I could include #ifdef's to
>> allow users to switch between row and column major. Templating
>> SkMatrix44 is another option -- something like SkMatrix44<typename T,
>> bool RowMajor> (possibly with other template arguments). This would
>> allow the user specify the storage, and would allow more than one flavor
>> of SkMatrix44 to be used in the same compilation unit. I don't think
>> that there's another storage scheme other than a dense array would be
>> sensible for a 4x4 matrix, so in terms of storage it seems like the only
>> decision the user needs to make is whether or not the matrix should be
>> row major -- please let me know if I've got that wrong.
>>
>> If we can let the user customize the storage of the matrix, would you be
>> more open to exposing a raw pointer to the data?
>>
>> http://codereview.appspot.com/4579050/
>>
>

[TomH, 2012-02-29 15:12:52]

This CL hasn't seen any action in 8 months, and I think Mike's comments are
effectively an r-. Can we close it?