[Dawn] Add multiple color-attachment support by sarahM0 · Pull Request #594 · google/amber

sarahM0 · 2019-07-23T20:24:33Z

fixs #547 and simplifies DoClear by using helper functions

dneto0

Do any tests start passing for Dawn, with this change?

src/dawn/engine_dawn.cc

dneto0 · 2019-07-24T17:54:26Z

src/dawn/engine_dawn.cc

+    ::dawn::Buffer copy_buffer = device.CreateBuffer(&descriptor);
+
+    ::dawn::BufferCopyView copy_buffer_view =
+        CreateBufferCopyView(copy_buffer, 0, dawn_row_pitch, 0);


Seems you can pull the creation of the buffer and the buffer view outside the the loop. Their parameters are independent of i.

dneto0 · 2019-07-24T17:56:43Z

src/dawn/engine_dawn.cc

+    if (!mapped_device_texture.result.IsSuccess())
+      return mapped_device_texture.result;
+
+    if (i < render_pipeline.pipeline->GetColorAttachments().size()) {


This is a bit weird. Why is the for loop at line 581 up to kMaxColorAttachments but then we only restrict this later part to colour attachments attached to this pipeline? It seems weird that we would be copying buffers back to the host when they are not attached to the pipeline and hence not accessible to the pipeline.??

Fair point!

i has not changed since it was bounds-checked at line 596. So this "if" should disappear. Its condition always evaluates to true. Replace this if construct by its "then" body.

src/dawn/engine_dawn.cc

dneto0 · 2019-07-24T18:05:03Z

src/dawn/engine_dawn.cc

-  rpd.colorAttachmentCount = 1;
-  rpd.colorAttachments = color_attachment_descriptor;
-  rpd.depthStencilAttachment = depth_stencil_descriptor;
+  DawnPipelineHelper helper;


This whole routine is a lot simpler now! :-)

sarahM0 · 2019-07-24T18:37:38Z

../amber/tests/cases/draw_rect_multiple_color_attachment.amber
is the test that passes now (I'd forgotten to add run_tests.py).

src/dawn/engine_dawn.cc

dneto0 · 2019-07-24T20:59:15Z

src/dawn/engine_dawn.cc

+    if (!mapped_device_texture.result.IsSuccess())
+      return mapped_device_texture.result;
+
+    if (i < render_pipeline.pipeline->GetColorAttachments().size()) {


i has not changed since it was bounds-checked at line 596. So this "if" should disappear. Its condition always evaluates to true. Replace this if construct by its "then" body.

dj2 · 2019-07-24T23:57:05Z

tests/cases/draw_rect_multiple_color_attachment.amber


 RUN my_pipeline DRAW_RECT POS 0 0 SIZE 800 600
-EXPECT framebuffer IDX 0 0 SIZE 800 600 EQ_RGBA 0 128 0 204
+EXPECT framebuffer IDX 0 0 SIZE 800 600 EQ_RGBA 0 127 0 204


Hm, I think this will fail on a vulkan host now (as it worked previously). Is this just a numerical difference between the dawn and vulkan engines?

I see your point however right now Vulkan fails with 128 and passes with 127, at least on my machine. Is there any reasons for this to behave differently on different machines?

Maybe? But, if both Vulkan and Dawn engines pass with this on your machine then I'm good with it. Maybe add a comment that we flipped the 128 to a 127 and if folks see this failing we need to find a better solution than changing the number? Just so we don't end up constantly flipping it back and forth, heh.

How about changing 0.5 to 0.499 to eliminate the confusion?

If that works, go for it.

Yeah, I suspect something funny with quantization of the very-low-precision floating point used in rendering. I could see it flipping back and forth.

All the other AmberScript scripts in the tests/cases directory use 127, except for the clear_color.amber script, but it's using an explicit 128 in that component of the clear color and it's using a UNORM so that's right in the middle of the range.

In short, I'm ok with this change. But let's keep an eye on this as an issue coming up.

FYI. This is what Vulkan says about how UNORM and SNORM are treated

https://www.khronos.org/registry/vulkan/specs/1.1/html/vkspec.html#fundamentals-fpfixedconv

I particular "Implementations should round to nearest. If r is equal to an integer, then that integer value must be returned."
So we could put 127.0/255.0 and it should get it. (that's 0.4980 while 128/255 is .50196) So 0.499 should be sufficient.

dneto0

For what it's worth, I ran this version of the test, and it passes on my old NV Quadro K2000 desktop GPU. It used to fail with the 128 expectation.

dneto0 · 2019-07-25T14:36:05Z

tests/cases/draw_rect_multiple_color_attachment.amber


 RUN my_pipeline DRAW_RECT POS 0 0 SIZE 800 600
-EXPECT framebuffer IDX 0 0 SIZE 800 600 EQ_RGBA 0 128 0 204
+EXPECT framebuffer IDX 0 0 SIZE 800 600 EQ_RGBA 0 127 0 204


FYI. This is what Vulkan says about how UNORM and SNORM are treated

https://www.khronos.org/registry/vulkan/specs/1.1/html/vkspec.html#fundamentals-fpfixedconv

dneto0 · 2019-07-25T14:38:51Z

tests/cases/draw_rect_multiple_color_attachment.amber


 RUN my_pipeline DRAW_RECT POS 0 0 SIZE 800 600
-EXPECT framebuffer IDX 0 0 SIZE 800 600 EQ_RGBA 0 128 0 204
+EXPECT framebuffer IDX 0 0 SIZE 800 600 EQ_RGBA 0 127 0 204


I particular "Implementations should round to nearest. If r is equal to an integer, then that integer value must be returned."
So we could put 127.0/255.0 and it should get it. (that's 0.4980 while 128/255 is .50196) So 0.499 should be sufficient.

init

1469d01

sarahM0 force-pushed the multi_color branch from a21b34b to 1469d01 Compare July 23, 2019 20:25

sarahM0 and others added 3 commits July 23, 2019 16:25

Merge branch 'master' into multi_color

888bb10

nit

202e284

Merge branch 'multi_color' of github.com:sarahM0/amber into multi_color

714c4ba

sarahM0 changed the title ~~[WIP] [Dawn] Add multicolor attachment support~~ [Dawn] Add multicolor attachment support Jul 23, 2019

sarahM0 changed the title ~~[Dawn] Add multicolor attachment support~~ [Dawn] Add multiple color-attachment support Jul 23, 2019

sarahM0 requested a review from dneto0 July 23, 2019 21:26

add is sequential error

d8ba52a

dneto0 requested changes Jul 24, 2019

View reviewed changes

review comments

f881d30

Merge branch 'master' into multi_color

d79c04a

sarahM0 requested a review from dneto0 July 24, 2019 19:58

dneto0 requested changes Jul 24, 2019

View reviewed changes

sarahM0 added 3 commits July 24, 2019 17:09

remove extra if/scope

d1cb1ec

Merge branch 'master' of github.com:google/amber into multi_color

333afbd

Merge branch 'multi_color' of github.com:sarahM0/amber into multi_color

9ed883b

dj2 reviewed Jul 24, 2019

View reviewed changes

sarahM0 and others added 3 commits July 25, 2019 09:44

Merge branch 'master' into multi_color

af8824f

0.499

4c2c213

Merge branch 'multi_color' of github.com:sarahM0/amber into multi_color

6f824a2

sarahM0 merged commit 03f8d9a into google:master Jul 25, 2019

sarahM0 deleted the multi_color branch July 25, 2019 16:28

dneto0 reviewed Jul 25, 2019

View reviewed changes

sarahM0 added a commit to sarahM0/amber that referenced this pull request Jul 25, 2019

[Dawn] Add multiple color-attachment support (google#594)

9eec366

Conversation

sarahM0 commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dneto0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahM0 commented Jul 24, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahM0 Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dneto0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sarahM0 commented Jul 23, 2019 •

edited

Loading

sarahM0 Jul 25, 2019 •

edited

Loading