Issue 147237: Adding GetStreamInfo functionality (and passing corresponding unit test).

Issue 147237: Adding GetStreamInfo functionality (and passing corresponding unit test). (Closed)

Created:
11 years, 6 months ago by rlp

Modified:
9 years, 7 months ago

Reviewers:
Antoine Labour, apatrick

CC:
o3d-review_googlegroups.com

Base URL:
svn://chrome-svn/chrome/trunk/src/o3d/

Visibility:
Public.

Description

Adding GetStreamInfo functionality (and passing corresponding unit test). Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19859

Patch Set 7 : '' #

Total comments: 3

Created: 11 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+384 lines, -5 lines)			Patch
M	command_buffer/client/cross/effect_helper.h	View	1 2 3 4 5	2 chunks	+21 lines, -0 lines	0 comments	Download
M	command_buffer/client/cross/effect_helper.cc	View	1 2 3 4 5	1 chunk	+63 lines, -0 lines	0 comments	Download
M	command_buffer/common/cross/cmd_buffer_format.h	View	1 2 3 4 5	1 chunk	+2 lines, -0 lines	0 comments	Download
M	command_buffer/common/cross/gapi_interface.h	View	1 2 3 4 5	1 chunk	+28 lines, -0 lines	0 comments	Download
M	command_buffer/common/cross/resource.h	View	1 2 3 4 5	1 chunk	+7 lines, -0 lines	0 comments	Download
M	command_buffer/service/cross/gapi_decoder.cc	View	1 2 3 4 5	1 chunk	+25 lines, -0 lines	0 comments	Download
M	command_buffer/service/cross/resource.h	View	1 2 3 4 5	1 chunk	+7 lines, -0 lines	1 comment	Download
M	command_buffer/service/win/d3d9/d3d9_utils.h	View	1 2 3 4 5	1 chunk	+53 lines, -0 lines	0 comments	Download
M	command_buffer/service/win/d3d9/effect_d3d9.h	View	1 2 3 4 5	3 chunks	+12 lines, -1 line	1 comment	Download
M	command_buffer/service/win/d3d9/effect_d3d9.cc	View	1 2 3 4 5 6	7 chunks	+96 lines, -2 lines	1 comment	Download
M	command_buffer/service/win/d3d9/gapi_d3d9.h	View	1 2 3 4 5	1 chunk	+11 lines, -0 lines	0 comments	Download
M	core/cross/command_buffer/effect_cb.h	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	core/cross/command_buffer/effect_cb.cc	View	1 2 3 4 5	3 chunks	+58 lines, -1 line	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Antoine Labour

http://codereview.chromium.org/147237/diff/3040/3045 File command_buffer/service/win/d3d9/effect_d3d9.cc (right): http://codereview.chromium.org/147237/diff/3040/3045#newcode94 Line 94: fs_constant_table_->Release(); I think you need to release d3d_vertex_shader_ ...

11 years, 5 months ago (2009-06-30 02:55:41 UTC) #2

http://codereview.chromium.org/147237/diff/3040/3045
File command_buffer/service/win/d3d9/effect_d3d9.cc (right):

http://codereview.chromium.org/147237/diff/3040/3045#newcode94
Line 94: fs_constant_table_->Release();
I think you need to release d3d_vertex_shader_ here

http://codereview.chromium.org/147237/diff/3040/3045#newcode328
Line 328: EffectStreamD3D9::Create(this, semantic, semantic_index);
You should only create the stream if D3DSemanticToCBSemantic returns success.

http://codereview.chromium.org/147237/diff/3040/3045#newcode360
Line 360: effect_->LinkStream(this);
I don't think you really need that complexity about LinkStream/UnlinkStream. It
is needed for effect parameters, because they can be referenced after the effect
has been destroyed (they have a resource ID), but that's not the case for
streams: you don't "create" them from the client side like you do to effects.
It'd be better to simply add them to have EffectStreamD3D9::Create just create
it and not call LinkStream, but have EffectD3D::SetStream to add it to the list.
Also, as it is right now, the EffectStreamD3D9 never get deleted.
Lastly, you could simply keep a list of effect_stream::Desc (not pointers to
EffectStreamD3D9), so that you don't have extra memory management to do. It has
all the data you really need to keep around.

http://codereview.chromium.org/147237/diff/3040/3045#newcode679
Line 679: if (!effect || size < sizeof(Uint32))  // NOLINT
size < sizeof(Uint32) is a bit irrelevant, since you're testing against
sizeof(effect_stream::Desc) later on and fail anyway.
It's needed for effect params because we need to be able to at least return the
size (which is a Uint32, and the first field of the effect_param::Desc struct),
but for streams, it's all fixed size.

http://codereview.chromium.org/147237/diff/3040/3041
File core/cross/command_buffer/effect_cb.cc (right):

http://codereview.chromium.org/147237/diff/3040/3041#newcode259
Line 259: array->push_back(EffectStreamInfo(semantic, semantic_index));
Don't add it to the array if CBSemanticToO3DSemantic fails.

apatrick

http://codereview.chromium.org/147237/diff/3040/3053 File command_buffer/client/cross/effect_helper.h (right): http://codereview.chromium.org/147237/diff/3040/3053#newcode128 Line 128: // array. These will be retrieved as many ...

11 years, 5 months ago (2009-06-30 17:12:49 UTC) #3

rlp

11 years, 5 months ago (2009-07-01 18:39:54 UTC) #4

http://codereview.chromium.org/147237/diff/3040/3053
File command_buffer/client/cross/effect_helper.h (right):

http://codereview.chromium.org/147237/diff/3040/3053#newcode128
Line 128: // array. These will be retrieved as many as posible at a time. At
least
On 2009/06/30 17:12:49, apatrick wrote:
> posible -> possible

Done.

http://codereview.chromium.org/147237/diff/3040/3044
File command_buffer/service/cross/resource.h (right):

http://codereview.chromium.org/147237/diff/3040/3044#newcode158
Line 158: explicit EffectStream() {}
On 2009/06/30 17:12:49, apatrick wrote:
> explicit is redundant

Done.

http://codereview.chromium.org/147237/diff/3040/3045
File command_buffer/service/win/d3d9/effect_d3d9.cc (right):

http://codereview.chromium.org/147237/diff/3040/3045#newcode94
Line 94: fs_constant_table_->Release();
On 2009/06/30 02:55:41, Antoine Labour wrote:
> I think you need to release d3d_vertex_shader_ here

Done.

http://codereview.chromium.org/147237/diff/3040/3045#newcode162
Line 162: HR(device->CreateVertexShader(
On 2009/06/30 17:12:49, apatrick wrote:
> I think it would be better to test the success of CreateVertexShader by
checking
> the return code:
> 
> if (HR(device->CreateVertexShader(

The HR macro takes care of logging D3D errors but doesn't actually return
anything so that check won't work. I'd rather keep the all the error checking
consistent for now.

http://codereview.chromium.org/147237/diff/3040/3045#newcode165
Line 165: DLOG(ERROR) << "Failed to create vertex shader";
On 2009/06/30 17:12:49, apatrick wrote:
> You might also want to release the effect and shader constant table otherwise
> they will leak I think.

Done.

http://codereview.chromium.org/147237/diff/3040/3045#newcode166
Line 166: return false;
On 2009/06/30 17:12:49, apatrick wrote:
> return NULL;

Done.

http://codereview.chromium.org/147237/diff/3040/3045#newcode328
Line 328: EffectStreamD3D9::Create(this, semantic, semantic_index);
On 2009/06/30 02:55:41, Antoine Labour wrote:
> You should only create the stream if D3DSemanticToCBSemantic returns success.

Done.

http://codereview.chromium.org/147237/diff/3040/3045#newcode360
Line 360: effect_->LinkStream(this);
On 2009/06/30 02:55:41, Antoine Labour wrote:
> I don't think you really need that complexity about LinkStream/UnlinkStream.
It
> is needed for effect parameters, because they can be referenced after the
effect
> has been destroyed (they have a resource ID), but that's not the case for
> streams: you don't "create" them from the client side like you do to effects.
> It'd be better to simply add them to have EffectStreamD3D9::Create just create
> it and not call LinkStream, but have EffectD3D::SetStream to add it to the
list.
> Also, as it is right now, the EffectStreamD3D9 never get deleted.
> Lastly, you could simply keep a list of effect_stream::Desc (not pointers to
> EffectStreamD3D9), so that you don't have extra memory management to do. It
has
> all the data you really need to keep around.

Done.

http://codereview.chromium.org/147237/diff/3040/3045#newcode679
Line 679: if (!effect || size < sizeof(Uint32))  // NOLINT
On 2009/06/30 02:55:41, Antoine Labour wrote:
> size < sizeof(Uint32) is a bit irrelevant, since you're testing against
> sizeof(effect_stream::Desc) later on and fail anyway.
> It's needed for effect params because we need to be able to at least return
the
> size (which is a Uint32, and the first field of the effect_param::Desc
struct),
> but for streams, it's all fixed size.

Done.

http://codereview.chromium.org/147237/diff/3040/3046
File command_buffer/service/win/d3d9/effect_d3d9.h (right):

http://codereview.chromium.org/147237/diff/3040/3046#newcode67
Line 67: // Resets the effect bresack-pointer. This is called when the effect
gets
On 2009/06/30 17:12:49, apatrick wrote:
> typo?

Done.

http://codereview.chromium.org/147237/diff/3040/3041
File core/cross/command_buffer/effect_cb.cc (right):

http://codereview.chromium.org/147237/diff/3040/3041#newcode259
Line 259: array->push_back(EffectStreamInfo(semantic, semantic_index));
On 2009/06/30 02:55:41, Antoine Labour wrote:
> Don't add it to the array if CBSemanticToO3DSemantic fails.

Done.

Antoine Labour

http://codereview.chromium.org/147237/diff/2036/3108 File command_buffer/service/cross/resource.h (right): http://codereview.chromium.org/147237/diff/2036/3108#newcode156 Line 156: class EffectStream: public Resource { This class is ...

11 years, 5 months ago (2009-07-06 23:48:11 UTC) #6

rlp

11 years, 5 months ago (2009-07-07 00:05:29 UTC) #7

This is already got committed. I'll create a new CL to fix these items.
- Rachel

On Mon, Jul 6, 2009 at 4:48 PM, <piman@google.com> wrote:

>
> http://codereview.chromium.org/147237/diff/2036/3108
> File command_buffer/service/cross/resource.h (right):
>
> http://codereview.chromium.org/147237/diff/2036/3108#newcode156
> Line 156: class EffectStream: public Resource {
> This class is not needed any more.
>
> http://codereview.chromium.org/147237/diff/2036/3109
> File command_buffer/service/win/d3d9/effect_d3d9.cc (right):
>
> http://codereview.chromium.org/147237/diff/2036/3109#newcode358
> Line 358: desc->semantic_index = stream.semantic_index;
> how about simply *desc = streams_[index]; instead of lines 354, 356-358
>
> http://codereview.chromium.org/147237/diff/2036/3110
> File command_buffer/service/win/d3d9/effect_d3d9.h (right):
>
> http://codereview.chromium.org/147237/diff/2036/3110#newcode131
> Line 131: friend class EffectStreamD3D9;
> Not needed any more.
>
>
> http://codereview.chromium.org/147237
>

Expand Messages | Collapse Messages