Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
135273008:
cc: Use ResourceProvider::Resouce::type to check the type of the given resource. (Closed)
|
Created:
6 years, 10 months ago by dshwang Modified:
6 years, 10 months ago CC:
chromium-reviews, cc-bugs_chromium.org, jbauman Base URL:
https://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptioncc: Use ResourceProvider::Resouce::type to check the type of the given resource.
Currently, some code uses the type variable to check the type of
the given resource, while other code uses the shared_bitmap,
pixels or mailbox variable to check the same condition.
This CL makes all code use the type variable for this purpose.
In addition, this CL clarify texture_pool can be non-null only if
the type is Texture and the origin is Internal.
In addition, add several DCHECKs to increase readability.
BUG=337519
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248861
Patch Set 1 #Patch Set 2 : Add DCHECK #
Total comments: 1
Patch Set 3 : Restore ResourceProvider::GetSharedMemory() #Patch Set 4 : Change CL's purpose #
Total comments: 17
Patch Set 5 : In addition, clarify texture_pool #
Total comments: 10
Patch Set 6 : Improve DCHECKs as reviewer suggested. #Patch Set 7 : Move Origin enum to better place :) #
Total comments: 6
Patch Set 8 : remove redundant comment. rebase to upstream #Patch Set 9 : remove redundant comment. rebase to upstream #Patch Set 10 : upload by intel account #
Total comments: 9
Patch Set 11 : Improve about DCHECK(pixels) #
Total comments: 5
Patch Set 12 : remove trailing comma in one line enum #
Total comments: 1
Patch Set 13 : rebase to upstream #
Messages
Total messages: 62 (0 generated)
https://codereview.chromium.org/135273008/diff/20001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/20001/cc/resources/resource_pr... cc/resources/resource_provider.cc:273: shared_bitmap(bitmap) { shared_bitmap can be NULL, while pixels must not be NULL.
> In addition, remove unused ResourceProvider::GetSharedMemory() method. This is because the software ubercomp path is still be worked on. Support was added to the resource provider but is not used yet. I would like to hold off on this change until we have software uber going, as there is likely going to be a bunch of flux (and definitely some code deletion) in the process.
On 2014/01/28 16:38:02, danakj wrote: > > In addition, remove unused ResourceProvider::GetSharedMemory() method. > > This is because the software ubercomp path is still be worked on. Support was > added to the resource provider but is not used yet. > > I would like to hold off on this change until we have software uber going, as > there is likely going to be a bunch of flux (and definitely some code deletion) > in the process. Thank you for explanation. I roll back ResourceProvider::GetSharedMemory(). Could I ask what "hold off on this change" mean? either this CL or removing ResourceProvider::GetSharedMemory()?
On 2014/01/28 17:40:03, dshwang wrote: > On 2014/01/28 16:38:02, danakj wrote: > > > In addition, remove unused ResourceProvider::GetSharedMemory() method. > > > > This is because the software ubercomp path is still be worked on. Support was > > added to the resource provider but is not used yet. > > > > I would like to hold off on this change until we have software uber going, as > > there is likely going to be a bunch of flux (and definitely some code > deletion) > > in the process. > > Thank you for explanation. I roll back ResourceProvider::GetSharedMemory(). > > Could I ask what "hold off on this change" mean? either this CL or removing > ResourceProvider::GetSharedMemory()? The CL. Since the "type" field is tied pretty tightly to the concept of hardware vs software resources, which we'll see work around in the next little bit. GetSharedMemory should be used soon by this work. If the type field is still redundant once we have the software working (it probably will be), then this will be great to do.
On 2014/01/28 17:50:03, danakj wrote: > The CL. Since the "type" field is tied pretty tightly to the concept of hardware > vs software resources, which we'll see work around in the next little bit. > GetSharedMemory should be used soon by this work. > > If the type field is still redundant once we have the software working (it > probably will be), then this will be great to do. Thank you for explanation. I did not know software compositor is working progress. I change this CL's goal, because there are some code which makes it easier to understand ResourceProvider's implementation.
https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:278: DCHECK(pixels); This is a great DCHECK. Since we don't change the pixels member except during delete, do you think this could cover for all the other DCHECK(pixels) you've added? https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:578: DCHECK(resource->type == Bitmap); DCHECK_EQ would be better here https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:615: if (resource->type == GLTexture) { Some of these changes look suspicious as LazyCreate can early out and give no gl_id when the texture pool is 0. But that looks to me like another check for hardware vs software. Can we do a patch for that first? Specifically: // Early out for resources that don't require texture creation. if (resource->texture_pool == 0) return; Can you verify that this is actually checking for "is this a software resource" and adjust that check if so? That'll give me more confidence for changes of this type here. https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:629: } else if (resource->type == Bitmap) { how about just else { DCHECK_EQ(resource->type, Bitmap); ... } Then you don't need a NOTREACHED. https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1282: DCHECK(source->shared_bitmap); If the shared bitmap manager is null, then this isn't true, right? Currently the shared bitmap manager is only non-null in unit tests. Eventually it will always be non-null. https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1379: DCHECK(!resource.shared_bitmap); I think you can drop this dcheck? It seems out of place here. https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1431: } else if (resource->type == Bitmap) { Can do the same here: } else { DCHECK_EQ(resource->type, Bitmap) ... } https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1470: } else if (resource->type == Bitmap) { and here } else { DCHECK_EQ(... https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1499: } else if (resource->type == Bitmap) { and here. actually here you don't need an else {} clause at all since the if {} does a return.
Thank you for review. I'll address your review soon! https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:278: DCHECK(pixels); On 2014/01/28 19:21:22, danakj wrote: > This is a great DCHECK. Since we don't change the pixels member except during > delete, do you think this could cover for all the other DCHECK(pixels) you've > added? Yes, currently, if type is Bitmap, pixels must not be null. and if type is GLTexture, pixels must be null. I think this constraint is hard to be changed. So I need to remove my other DCHECK(resource->pixels) not to make us confused. https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:578: DCHECK(resource->type == Bitmap); On 2014/01/28 19:21:22, danakj wrote: > DCHECK_EQ would be better here good advice. I'll do it. https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:615: if (resource->type == GLTexture) { On 2014/01/28 19:21:22, danakj wrote: > Some of these changes look suspicious as LazyCreate can early out and give no > gl_id when the texture pool is 0. But that looks to me like another check for > hardware vs software. > > Can we do a patch for that first? > > Specifically: > > // Early out for resources that don't require texture creation. > > if (resource->texture_pool == 0) > return; > > Can you verify that this is actually checking for "is this a software resource" > and adjust that check if so? That'll give me more confidence for changes of this > type here. resource->texture_pool is always non-null only if origin is Internal and type is GLTexture. Otherwise, always null. So the if statement can change to if (!(resource->type == GLTexture && resource->origin == Internal)) return; I'll add above comment to new patch set. https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:629: } else if (resource->type == Bitmap) { On 2014/01/28 19:21:22, danakj wrote: > how about just > > else { > DCHECK_EQ(resource->type, Bitmap); > ... > } > > Then you don't need a NOTREACHED. yes! https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1282: DCHECK(source->shared_bitmap); On 2014/01/28 19:21:22, danakj wrote: > If the shared bitmap manager is null, then this isn't true, right? > > Currently the shared bitmap manager is only non-null in unit tests. Eventually > it will always be non-null. That means this line had that bug, and this change fixes it? https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1379: DCHECK(!resource.shared_bitmap); On 2014/01/28 19:21:22, danakj wrote: > I think you can drop this dcheck? It seems out of place here. yes, i see. this dcheck is always true, but not very meaningful.
https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1282: DCHECK(source->shared_bitmap); On 2014/01/28 19:48:13, dshwang wrote: > On 2014/01/28 19:21:22, danakj wrote: > > If the shared bitmap manager is null, then this isn't true, right? > > > > Currently the shared bitmap manager is only non-null in unit tests. Eventually > > it will always be non-null. > > That means this line had that bug, and this change fixes it? Hm, I guess we don't transfer resources without a shared_bitmap, so this is okay.
In new patch set, I clarify texture_pool can be non-null only if the type is Texture and the origin is Internal. https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1282: DCHECK(source->shared_bitmap); On 2014/01/28 20:05:38, danakj wrote: > Hm, I guess we don't transfer resources without a shared_bitmap, so this is > okay. yes, i think so. currently, only unittests delegates software resources to the parent.
https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:240: DCHECK(texture_pool || origin != Internal); For this DCHECK, I change the constructor. https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1568: if (resource->type == GLTexture) { For this change, I changed ResourceProvider::LazyCreate() https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1675: DCHECK(resource->texture_pool); how about the change of LazyCreate()?
https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:240: DCHECK(texture_pool || origin != Internal); On 2014/01/28 20:35:36, dshwang wrote: > For this DCHECK, I change the constructor. DCHECK_EQ(origin == Internal, !!texture_pool) ? https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:438: GLuint texture_id) { Can we DCHECK that texture_id is not 0 here? https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:764: if (!resource->gl_id && resource->type == GLTexture) { can you reverse these 2 conditions? checking the gl_id doesn't even make sense if you're not GLTexture type. https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1295: DCHECK(source->shared_bitmap); given the next line will crash anyway, not sure we need this dcheck? https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1558: Should we DCHECK the resource has Internal Origin here? https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1668: DCHECK(!resource->texture_pool); I think these DCHECKs can be covered in the constructor as I suggest in another comment. https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/135273008/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.h:368: enum Origin { This seems like an odd place, in the middle of the constructors/destructors. Move to the top of the struct with a whitespace line after it?
On 2014/01/28 21:59:01, danakj wrote: Thank you for careful review! I address everything. Could you review again?
https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:281: DCHECK(pixels); Reviewing a software ubercomp CL (https://codereview.chromium.org/148243013), this will not be true for software resources that are received in a renderer process from a child renderer. The child does not get access to the pixels, so the current code will receive the resource and set the pixels field to NULL. So I guess it's better to DCHECK this in places that will actually access the pixels instead. https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:1297: // Currently, this routine is used by only unittests. I don't think you need this, we'll just have to remove it later.
thx for review. could you re-review? https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:281: DCHECK(pixels); On 2014/01/30 21:50:31, danakj wrote: > Reviewing a software ubercomp CL (https://codereview.chromium.org/148243013), > this will not be true for software resources that are received in a renderer > process from a child renderer. The child does not get access to the pixels, so > the current code will receive the resource and set the pixels field to NULL. > > So I guess it's better to DCHECK this in places that will actually access the > pixels instead. I still think pixel is always non-null. See below comment https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:1096: if ((!it->is_software && !gl) || (it->is_software && !pixels)) { if pixels is null, the child returns early instead of making Resource. I can not find where pixels can be non-null. Could you give me tip if i'm wrong. https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:1297: // Currently, this routine is used by only unittests. On 2014/01/30 21:50:31, danakj wrote: > I don't think you need this, we'll just have to remove it later. indeed, i removed it.
https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:1096: if ((!it->is_software && !gl) || (it->is_software && !pixels)) { On 2014/01/31 17:25:34, dshwang wrote: > if pixels is null, the child returns early instead of making Resource. > > I can not find where pixels can be non-null. Could you give me tip if i'm wrong. Oh, hm. I need to bring this up over there then. Cuz this will happen when we have multiple levels of delegation.
Currently, there are 3 places to call Resource(pixels, ...) constructor. As I understand your suggestion, I need to add DCHECK(pixels) in two places instead of in the constructor as below comments. Could you confirm if I understand correctly? https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:281: DCHECK(pixels); remove DCHECK(pixels); https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:429: pixels, bitmap.release(), size, Resource::Internal, GL_LINEAR, wrap_mode); add DCHECK(pixels); https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:501: GL_CLAMP_TO_EDGE); add DCHECK(pixels); https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:1112: GL_CLAMP_TO_EDGE); Don't add DCHECK(pixels);
Yeh, that sounds good, also a in places where we use the pixel_buffer, or map the pixels. https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:1444: if (resource->pixel_buffer) Also DCHECK(pixels) here. https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:1481: if (!resource->pixel_buffer) And here https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:1508: return resource->pixel_buffer; And here. https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:1607: DCHECK(!resource->mailbox.IsValid()); And here. https://codereview.chromium.org/135273008/diff/200001/cc/resources/resource_p... cc/resources/resource_provider.cc:1816: return resource->pixels; And here.
Thank you for review! Could you re-review? I guess this turn is final :) https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... cc/resources/resource_provider.cc:281: DCHECK(origin == Delegated || pixels); I replace this one DCHECK with your suggestion adding DCHECK in places where we use the pixel_buffer, or map the pixels. It is mainly because all the place checks if type is Bitmap and origin is Internal.
LGTM https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... cc/resources/resource_provider.cc:281: DCHECK(origin == Delegated || pixels); On 2014/02/03 12:05:12, dshwang wrote: > I replace this one DCHECK with your suggestion adding DCHECK in places where we > use the pixel_buffer, or map the pixels. > It is mainly because all the place checks if type is Bitmap and origin is > Internal. Ah, the Internal checks help out, good. https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... cc/resources/resource_provider.h:366: enum Origin { Internal, External, Delegated, }; nit: no trailing comma if they're all on one line
Thank you for review! https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... cc/resources/resource_provider.h:366: enum Origin { Internal, External, Delegated, }; On 2014/02/03 18:09:18, danakj wrote: > nit: no trailing comma if they're all on one line Without ";", compile fails as follows: ../../cc/resources/resource_provider.h:366:5: note: (perhaps a semicolon is missing after the definition of 'cc::ResourceProvider::Resource::Origin') It's not my preference. 4 days ago, this style rule is included.
The CQ bit was checked by dongseong.hwang@intel.com
On Mon, Feb 3, 2014 at 1:55 PM, <dongseong.hwang@intel.com> wrote: > Thank you for review! > > > > https://codereview.chromium.org/135273008/diff/320001/cc/ > resources/resource_provider.h > File cc/resources/resource_provider.h (right): > > https://codereview.chromium.org/135273008/diff/320001/cc/ > resources/resource_provider.h#newcode366 > cc/resources/resource_provider.h:366: enum Origin { Internal, External, > Delegated, }; > On 2014/02/03 18:09:18, danakj wrote: > >> nit: no trailing comma if they're all on one line >> > > Without ";", compile fails as follows: > ../../cc/resources/resource_provider.h:366:5: note: (perhaps a semicolon > is missing after the definition of > 'cc::ResourceProvider::Resource::Origin') > > It's not my preference. 4 days ago, this style rule is included. > Oh interesting. Okay :) > > https://codereview.chromium.org/135273008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/135273008/32...
On 2014/02/03 18:55:10, dshwang wrote: > Thank you for review! > > https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... > File cc/resources/resource_provider.h (right): > > https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... > cc/resources/resource_provider.h:366: enum Origin { Internal, External, > Delegated, }; > On 2014/02/03 18:09:18, danakj wrote: > > nit: no trailing comma if they're all on one line > > Without ";", compile fails as follows: > ../../cc/resources/resource_provider.h:366:5: note: (perhaps a semicolon is > missing after the definition of 'cc::ResourceProvider::Resource::Origin') > > It's not my preference. 4 days ago, this style rule is included. There are many one line enums. $ git grep " enum.*}" ... cc/animation/animation_curve.h: enum CurveType { Color, Float, Transform, Filter, ScrollOffset }; cc/animation/animation_events.h: enum Type { Started, Finished, Aborted, PropertyUpdate }; cc/input/input_handler.h: enum ScrollStatus { ScrollOnMainThread, ScrollStarted, ScrollIgnored }; cc/input/input_handler.h: enum ScrollInputType { Gesture, Wheel, NonBubblingGesture }; cc/resources/managed_tile_state.h: enum Mode { RESOURCE_MODE, SOLID_COLOR_MODE, PICTURE_PILE_MODE }; cc/resources/picture_layer_tiling.cc: enum { BOTTOM, TOP, LEFT, RIGHT } edge; cc/resources/pixel_buffer_raster_worker_pool.h: enum RasterTaskState { UNSCHEDULED, SCHEDULED, UPLOADING, COMPLETED }; ...
https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... cc/resources/resource_provider.h:366: enum Origin { Internal, External, Delegated, }; On 2014/02/03 18:55:11, dshwang wrote: > On 2014/02/03 18:09:18, danakj wrote: > > nit: no trailing comma if they're all on one line > > Without ";", compile fails as follows: > ../../cc/resources/resource_provider.h:366:5: note: (perhaps a semicolon is > missing after the definition of 'cc::ResourceProvider::Resource::Origin') > > It's not my preference. 4 days ago, this style rule is included. Oh.. I meant the , not the ; { Internal, External, Delegated }
The CQ bit was unchecked by dongseong.hwang@intel.com
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
On 2014/02/03 18:58:51, danakj wrote: > https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... > File cc/resources/resource_provider.h (right): > > https://codereview.chromium.org/135273008/diff/320001/cc/resources/resource_p... > cc/resources/resource_provider.h:366: enum Origin { Internal, External, > Delegated, }; > On 2014/02/03 18:55:11, dshwang wrote: > > On 2014/02/03 18:09:18, danakj wrote: > > > nit: no trailing comma if they're all on one line > > > > Without ";", compile fails as follows: > > ../../cc/resources/resource_provider.h:366:5: note: (perhaps a semicolon is > > missing after the definition of 'cc::ResourceProvider::Resource::Origin') > > > > It's not my preference. 4 days ago, this style rule is included. > > Oh.. I meant the , not the ; > > { Internal, External, Delegated } aha, I see. I'll apply it soon :)
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/135273008/44...
https://codereview.chromium.org/135273008/diff/440002/cc/resources/resource_p... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/135273008/diff/440002/cc/resources/resource_p... cc/resources/resource_provider.h:366: enum Origin { Internal, External, Delegated }; I changed it. Thank you for detail review. I learned a lot :)
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for cc/resources/resource_provider.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file cc/resources/resource_provider.cc
Hunk #14 FAILED at 559.
Hunk #19 FAILED at 1105.
Hunk #20 succeeded at 1122 with fuzz 1 (offset 2 lines).
Hunk #21 FAILED at 1289.
Hunk #22 succeeded at 1359 (offset 5 lines).
Hunk #23 succeeded at 1389 (offset 5 lines).
Hunk #24 succeeded at 1440 (offset 5 lines).
Hunk #25 succeeded at 1477 (offset 5 lines).
Hunk #26 succeeded at 1505 (offset 5 lines).
Hunk #27 succeeded at 1558 (offset 5 lines).
Hunk #28 succeeded at 1567 (offset 5 lines).
Hunk #29 succeeded at 1603 (offset 5 lines).
Hunk #30 succeeded at 1666 (offset 5 lines).
Hunk #31 succeeded at 1806 (offset 5 lines).
3 out of 31 hunks FAILED -- saving rejects to file
cc/resources/resource_provider.cc.rej
Patch: cc/resources/resource_provider.cc
Index: cc/resources/resource_provider.cc
diff --git a/cc/resources/resource_provider.cc
b/cc/resources/resource_provider.cc
index
8b9453fc1b55bbd9145f8cdf6ccec734e72b30fb..330c12f693d4496c73b43f0b43078749f34e6f36
100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -190,7 +190,7 @@ ResourceProvider::Resource::Resource()
wrap_mode(0),
lost(false),
hint(TextureUsageAny),
- type(static_cast<ResourceType>(0)),
+ type(InvalidType),
format(RGBA_8888),
shared_bitmap(NULL) {}
@@ -198,6 +198,7 @@ ResourceProvider::Resource::~Resource() {}
ResourceProvider::Resource::Resource(GLuint texture_id,
const gfx::Size& size,
+ Origin origin,
GLenum target,
GLenum filter,
GLenum texture_pool,
@@ -214,7 +215,7 @@ ResourceProvider::Resource::Resource(GLuint texture_id,
imported_count(0),
exported_count(0),
locked_for_write(false),
- origin(Internal),
+ origin(origin),
marked_for_deletion(false),
pending_set_pixels(false),
set_pixels_completion_forced(false),
@@ -236,11 +237,13 @@ ResourceProvider::Resource::Resource(GLuint texture_id,
format(format),
shared_bitmap(NULL) {
DCHECK(wrap_mode == GL_CLAMP_TO_EDGE || wrap_mode == GL_REPEAT);
+ DCHECK_EQ(origin == Internal, !!texture_pool);
}
ResourceProvider::Resource::Resource(uint8_t* pixels,
SharedBitmap* bitmap,
const gfx::Size& size,
+ Origin origin,
GLenum filter,
GLint wrap_mode)
: child_id(0),
@@ -253,7 +256,7 @@ ResourceProvider::Resource::Resource(uint8_t* pixels,
imported_count(0),
exported_count(0),
locked_for_write(false),
- origin(Internal),
+ origin(origin),
marked_for_deletion(false),
pending_set_pixels(false),
set_pixels_completion_forced(false),
@@ -275,6 +278,7 @@ ResourceProvider::Resource::Resource(uint8_t* pixels,
format(RGBA_8888),
shared_bitmap(bitmap) {
DCHECK(wrap_mode == GL_CLAMP_TO_EDGE || wrap_mode == GL_REPEAT);
+ DCHECK(origin == Delegated || pixels);
}
ResourceProvider::Child::Child() : marked_for_deletion(false) {}
@@ -392,8 +396,15 @@ ResourceProvider::ResourceId
ResourceProvider::CreateGLTexture(
DCHECK(thread_checker_.CalledOnValidThread());
ResourceId id = next_id_++;
- Resource resource(
- 0, size, target, GL_LINEAR, texture_pool, wrap_mode, hint, format);
+ Resource resource(0,
+ size,
+ Resource::Internal,
+ target,
+ GL_LINEAR,
+ texture_pool,
+ wrap_mode,
+ hint,
+ format);
resource.allocated = false;
resources_[id] = resource;
return id;
@@ -412,10 +423,11 @@ ResourceProvider::ResourceId
ResourceProvider::CreateBitmap(
pixels = bitmap->pixels();
else
pixels = new uint8_t[4 * size.GetArea()];
+ DCHECK(pixels);
ResourceId id = next_id_++;
Resource resource(
- pixels, bitmap.release(), size, GL_LINEAR, wrap_mode);
+ pixels, bitmap.release(), size, Resource::Internal, GL_LINEAR,
wrap_mode);
resource.allocated = true;
resources_[id] = resource;
return id;
@@ -427,6 +439,8 @@ ResourceProvider::CreateResourceFromExternalTexture(
GLuint texture_id) {
DCHECK(thread_checker_.CalledOnValidThread());
+ DCHECK(texture_target);
+ DCHECK(texture_id);
GLES2Interface* gl = ContextGL();
DCHECK(gl);
GLC(gl, gl->BindTexture(texture_target, texture_id));
@@ -440,13 +454,13 @@ ResourceProvider::CreateResourceFromExternalTexture(
ResourceId id = next_id_++;
Resource resource(texture_id,
gfx::Size(),
+ Resource::External,
texture_target,
GL_LINEAR,
0,
GL_CLAMP_TO_EDGE,
TextureUsageAny,
RGBA_8888);
- resource.origin = Resource::External;
resource.allocated = true;
resources_[id] = resource;
return id;
@@ -463,6 +477,7 @@ ResourceProvider::ResourceId
ResourceProvider::CreateResourceFromTextureMailbox(
if (mailbox.IsTexture()) {
resource = Resource(0,
gfx::Size(),
+ Resource::External,
mailbox.target(),
GL_LINEAR,
0,
@@ -474,6 +489,7 @@ ResourceProvider::ResourceId
ResourceProvider::CreateResourceFromTextureMailbox(
base::SharedMemory* shared_memory = mailbox.shared_memory();
DCHECK(shared_memory->memory());
uint8_t* pixels = reinterpret_cast<uint8_t*>(shared_memory->memory());
+ DCHECK(pixels);
scoped_ptr<SharedBitmap> shared_bitmap;
if (shared_bitmap_manager_) {
shared_bitmap =
@@ -482,10 +498,10 @@ ResourceProvider::ResourceId
ResourceProvider::CreateResourceFromTextureMailbox(
resource = Resource(pixels,
shared_bitmap.release(),
mailbox.shared_memory_size(),
+ Resource::External,
GL_LINEAR,
GL_CLAMP_TO_EDGE);
}
- resource.origin = Resource::External;
resource.allocated = true;
resource.mailbox = mailbox;
resource.release_callback =
@@ -543,7 +559,8 @@ void
ResourceProvider::DeleteResourceInternal(ResourceMap::iterator it,
}
if (resource->mailbox.IsValid() && resource->origin == Resource::External) {
GLuint sync_point = resource->mailbox.sync_point();
- if (resource->mailbox.IsTexture()) {
+ if (resource->type == GLTexture) {
+ DCHECK(resource->mailbox.IsTexture());
lost_resource |= lost_output_surface_;
GLES2Interface* gl = ContextGL();
DCHECK(gl);
@@ -573,6 +590,7 @@ void
ResourceProvider::DeleteResourceInternal(ResourceMap::iterator it,
}
if (resource->shared_bitmap) {
DCHECK(resource->origin != Resource::External);
+ DCHECK_EQ(Bitmap, resource->type);
delete resource->shared_bitmap;
resource->pixels = NULL;
}
@@ -609,7 +627,8 @@ void ResourceProvider::SetPixels(ResourceId id,
DCHECK(ReadLockFenceHasPassed(resource));
LazyAllocate(resource);
- if (resource->gl_id) {
+ if (resource->type == GLTexture) {
+ DCHECK(resource->gl_id);
DCHECK(!resource->pending_set_pixels);
DCHECK_EQ(resource->target, static_cast<GLenum>(GL_TEXTURE_2D));
GLES2Interface* gl = ContextGL();
@@ -622,9 +641,8 @@ void ResourceProvider::SetPixels(ResourceId id,
dest_offset,
resource->format,
resource->size);
- }
-
- if (resource->pixels) {
+ } else {
+ DCHECK_EQ(Bitmap, resource->type);
DCHECK(resource->allocated);
DCHECK_EQ(RGBA_8888, resource->format);
SkBitmap src_full;
@@ -747,8 +765,9 @@ const ResourceProvider::Resource*
ResourceProvider::LockForRead(ResourceId id) {
LazyCreate(resource);
- if (!resource->gl_id && resource->mailbox.IsTexture()) {
+ if (resource->type == GLTexture && !resource->gl_id) {
DCHECK(resource->origin != Resource::Internal);
+ DCHECK(resource->mailbox.IsTexture());
GLES2Interface* gl = ContextGL();
DCHECK(gl);
if (resource->mailbox.sync_point()) {
@@ -1087,11 +1106,16 @@ void ResourceProvider::ReceiveFromChild(
ResourceId local_id = next_id_++;
Resource& resource = resources_[local_id];
if (it->is_software) {
- resource = Resource(
- pixels, bitmap.release(), it->size, GL_LINEAR, GL_CLAMP_TO_EDGE);
+ resource = Resource(pixels,
+ bitmap.release(),
+ it->size,
+ Resource::Delegated,
+ GL_LINEAR,
+ GL_CLAMP_TO_EDGE);
} else {
resource = Resource(0,
it->size,
+ Resource::Delegated,
it->target,
it->filter,
0,
@@ -1102,7 +1126,6 @@ void ResourceProvider::ReceiveFromChild(
TextureMailbox(it->mailbox, it->target, it->sync_point);
}
resource.child_id = child;
- resource.origin = Resource::Delegated;
// Don't allocate a texture for a child.
resource.allocated = true;
resource.imported_count = 1;
@@ -1272,7 +1295,7 @@ void ResourceProvider::TransferResource(GLES2Interface*
gl,
resource->filter = source->filter;
resource->size = source->size;
- if (source->shared_bitmap) {
+ if (source->type == Bitmap) {
resource->mailbox = source->shared…
(message too large)
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by dongseong.hwang@intel.com
The CQ bit was unchecked by dongseong.hwang@intel.com
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/135273008/82...
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/135273008/82...
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/135273008/82...
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/135273008/82...
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, sync_integration_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/135273008/82...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/135273008/82...
Message was sent while issue was closed.
Change committed as 248861 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
