704073 - AMF support for float and float4 and Vector.<float> and Vector.<float4>

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Lars T Hansen]

The built-in AMF support must also provide reading and writing of float and float4, and Vectors of those types.

Comment 1

[Sateesh]

Attachment: AMF support for float/float4 and their vectors

Code changes to support AMF serialization/deserialization for float, float4, vector.<float> and vector.<float4>.

The patch also has a test case AMFSerializer.as -- an existing test file modified to test the new types as well.

Comment 2

[Sateesh]

The just attached patch is, obviously, based on tr-float branch but not tamarin-redux.

Comment 3

[Virgil Palanciuc]

Comment on attachment 580387 [details] [diff] [review]
AMF support for float/float4 and their vectors

Review of attachment 580387 [details] [diff] [review]:
-----------------------------------------------------------------

Only two nits from me, looks good. I'm slightly concerned about Vector.<float4> since there's some magic Lars did there to align the elements, but since you use Float4VectorAccesor, it's probably working properly. I'm sure Lars will spot any potential issues there, if any :)

::: core/AvmSerializer.cpp
@@ +294,5 @@
> +#ifdef VMCFG_FLOAT
> +        else if(AvmCore::isFloat(value))
> +        {
> +            WriteType(kFloatAtomType);
> +            WriteFloat(AvmCore::singlePrecisionFloat(value));

I'm not a reviewer, but I'll just assume you don't mind the feedback :)

While AvmCore::singlePrecisionFloat() is not wrong, IMO it would be better (and clearer) to use here AvmCore::atomToFloat()

::: core/DataIO.cpp
@@ +117,5 @@
> +    {
> +        union {
> +            float b[4];
> +            float4_t f4;
> +        };

Why "union" here and "reinterpret_cast" in WriteFloat4? Should use the reinterpret_cast here, too. (we have found compilers to be buggy when using native/intrinsic float4_t types inside unions (e.g. __m128); an while currently we don't use the intrinsic type, IMO it's more likely that we'll use it in the future than it is that we'll enable strict aliasing in the future.

Comment 4

[Lars T Hansen]

Comment on attachment 580387 [details] [diff] [review]
AMF support for float/float4 and their vectors

I agree with Virgil's comments.  The code accessing the Vector.<float4> should be fine as it goes through the Float4VectorAccessor, which has been modified suitably.

I have a couple of nits/suggestions of my own:

In the test code you use "new float(...)" and "new float4(...)" rather than just "float(...)" and "float4(...)".  The forms are equivalent, but the latter forms are strongly preferred.

The float4 tests would be stronger if they did not use the same value in all four positions of the float4.

The tests would be stronger if they did not simply write and read a value, but if they inspected the bit representation of the ByteArray of the written representation and also hand-constructed a bit representation that would then be read, and checked that it was read correctly.  We've had ByteArray bugs before where write/read roundtripping did not catch the problem because the bug was in both paths.

Comment 5

[T S S S YADAV]

Hi Lars,

I am Sathish Yadav and I am responsible for writing the test cases for this bug.

I have added/modified test cases for your comment above, except for the last point that you have mentioned about inspecting ByteArray.write and ByteArray.read operations against a hand constructed bit representation of float/float4 types. As far as I know, I haven't come across acceptance test cases similar to this.

I wish to know if we can proceed with writing test cases(hand constructed bit representation) for float/float4 alone or subject all legacy types to test along these same lines.

I would also like to mention the cost of testing on these lines will be high and the test can be guaranteed air tight only by a suitable reviewer or a team of reviewers.

I request your thoughts on these before I proceed testing on these lines.

Comment 6

[T S S S YADAV]

Hi Lars,

Can you also suggest us the name(s) of reviewers for acceptance test that has been prepared for float/float4 changes?

Comment 7

[Lars T Hansen]

Hi Satish,

Acceptance test cases that construct bit representations and then read data, or write data and check the bit representation, can be found in various forms in test/acceptance/as3/ByteArray/ByteArray.as, though these tests are not as common as they arguably ought to be.  See for example the testUtf() case, which writes individual bytes and then tries to read those bytes as an UTF-8 encoded strings.

For a reviewer name please write to Trevor Baker (trbaker@adobe).

Comment 8

[Sateesh]

Attachment: Revision 1 for float/float4 AMF support

Virgil, thanks for reviewing. I have addressed your comments in this patch (rebased to the latest changes in the branch).

Lars, thanks for reviewing. the test case comments regarding new float and using different values for each of the floats in float4 are also addressed in this patch (a few test cases still use new float/float4 just to have a couple of them).

Comment 9

[Lars T Hansen]

Comment on attachment 580879 [details] [diff] [review]
Revision 1 for float/float4 AMF support

Rubber stamp.

Comment 10

[Trevor Baker]

Comment on attachment 580879 [details] [diff] [review]
Revision 1 for float/float4 AMF support

Brent, please review the testcases.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 580879 [details] [diff] [review]
Revision 1 for float/float4 AMF support

Review of attachment 580879 [details] [diff] [review]:
-----------------------------------------------------------------

::: core/DataIO.cpp
@@ +116,5 @@
> +    float4_t DataInput::ReadFloat4()
> +    {
> +        float4_t f4;
> +        float* b = reinterpret_cast<float*>(&f4);
> +

Question (which is largely directed at Virgil, not at Sateesh): could we encapsulate these casts in an inlined operator[] for float4_t ?  (Or are we concerned about compilers not CSE'ing the common parts of the inlined code well enough?)

@@ +266,5 @@
>      }
>  
> +    void DataOutput::WriteFloat4(float4_t value)
> +    {
> +        float* pval = reinterpret_cast<float*>(&value);

(see above note on offering an operator[] for float4_t.)

Comment 12

[Lars T Hansen]

(In reply to Felix S Klock II from comment #11)

> Question (which is largely directed at Virgil, not at Sateesh): could we
> encapsulate these casts in an inlined operator[] for float4_t ?  (Or are we
> concerned about compilers not CSE'ing the common parts of the inlined code
> well enough?)

I'd be super concerned about this overstepping the bounds of POD-data and what that might do to the generated code.

I agree the current casts are far from ideal and they were discussed when Virgil originally integrated the code; we decided that enough places depended on this working that it was OK, given how few places it's necessary to do this.

Comment 13

[Brent Baker]

Comment on attachment 580879 [details] [diff] [review]
Revision 1 for float/float4 AMF support

Review of attachment 580879 [details] [diff] [review]:
-----------------------------------------------------------------

AMFSerializer.as:
I would suggest modifying buildBAnCallAddTC() to not only check the values read out of the ByteArray but also check that the type of the input matches the extracted type from readObject. Something like:
	ba_test_Obj.position = 0;
	AddTestCase("Type check" + strTitle, typeof input, typeof ba_test_Obj.readObject());


I would also echo the comment from Lars that you should add some sort of testing that is not just round tripping of reading and writing. Something along this lines of this:

var f:float = 12.375f;
var ba_test_Obj:ByteArray = new ByteArray();
ba_test_Obj.writeObject(f);
ba_test_Obj.position = 0;
print(ba_test_Obj.readByte()); // should be: kFloatAtomType = 21
print(ba_test_Obj.readInt());  // 0-10000010-10001100000000000000000 binary rep
                               // should be: 1095106560 as int


ba_test_Obj.position = 0;
ba_test_Obj.writeByte(21);
ba_test_Obj.writeInt(1095106560);
ba_test_Obj.position = 0;
print(ba_test_Obj.readObject()); // should be 12.375
ba_test_Obj.position = 0;
print(typeof ba_test_Obj.readObject()); // should be float

Comment 14

[Sateesh]

(In reply to Brent Baker from comment #13)

Brent, thanks for reviewing plus for the detailed examples.

We also have a similar idea on how to do this (testing reading & writing paths independently) -- we could store the expected binary data in a ByteArray corresponding to the object we want to serialize/deserialize and then compare it with the actual serialized/deserialized data -- the ByteArray contents would have to be manually reviewed once.

However, this needs to be done for all types of objects, not just for float/float4 . So, is it okay to do all these changes under this bug?

Comment 15

[Lars T Hansen]

Please land it as soon as you have a chance and file mop-up bugs for remaining work.

Comment 16

[T S S S YADAV]

Hi Lars and Brent,

I have added test cases for type check, owing to the modularity of the code, I was able to add type checks for all types instead of just float/float4 alone.  

Do you want to me to submit this change or pull down my type checks to float/float4 alone?

Comment 17

[T S S S YADAV]

FYI, the following float4 members are returning undefined,

float4.MAX_VALUE
float4.MIN_VALUE
float4.POSITIVE_INFINITY
float4.NEGATIVE_INFINITY
float4.E
float4.LN10
float4.LN2
float4.LOG2E
float4.LOG10E
float4.PI
float4.SQRT1_2
float4.SQRT2

Although the test code is passing now because the expected and actual are undefined, I would like to know if anything should be done by us?

Comment 18

[T S S S YADAV]

I will be writing test cases to compare bit level comparisons of objects in AMF3 format.  

This set of additional test cases will cost us time and I would like to know if you have any timeline in mind?

Comment 19

[Lars T Hansen]

(In reply to T S S S YADAV from comment #17)
> FYI, the following float4 members are returning undefined,
> 
> float4.MAX_VALUE
> float4.MIN_VALUE
> float4.POSITIVE_INFINITY
> float4.NEGATIVE_INFINITY
> float4.E
> float4.LN10
> float4.LN2
> float4.LOG2E
> float4.LOG10E
> float4.PI
> float4.SQRT1_2
> float4.SQRT2
> 
> Although the test code is passing now because the expected and actual are
> undefined, I would like to know if anything should be done by us?

Those properties are no longer defined on float4 (nor are many methods that used to be defined on float4 there any longer).

Be sure to work from current specs.  They are available from Zerowing float/float4 project page, see the "architecture" section.

Comment 20

[Lars T Hansen]

Lars will land.

Comment 21

[Lars T Hansen]

changeset:   7096:eccc213966ad
tag:         tip
user:        Lars T Hansen <[e-mail]>
date:        Fri Dec 16 16:45:53 2011 +0100
summary:     AMF support for float and float4

Comment 22

[Lars T Hansen]

Additional testing should be filed as a follow-on bug.

Comment 23

[Brent Baker]

(In reply to Lars T Hansen from comment #21)
> changeset:   7096:eccc213966ad
> tag:         tip
> user:        Lars T Hansen <[e-mail]>
> date:        Fri Dec 16 16:45:53 2011 +0100
> summary:     AMF support for float and float4

We will actually need to duplicate the AMFSerializer.as testcase so that we can isolate the float/float4 code. This is necessary since we will be merging the tr-float repo into tamarin-redux with float disabled by default. I will work on a patch and attach to bug# 711370

Comment 24

[Brent Baker]

changeset:   7098:e165d2871835
tag:         tip
user:        Brent Baker <[e-mail]>
date:        Fri Dec 16 14:36:30 2011 -0500
summary:     Bug 704073: split out the float and float4 AMF testcases into a separate test file.

Comment 25

[Brent Baker]

Bug entered for adding additional float/float4 bit representation testing as bug# 704073

Comment 26

[Gavin]

Not working on this anymore.