Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
454843002:
cc: Do bitmap conversion for RasterBuffer in the worker thread. (Closed)
Descriptioncc: Do bitmap conversion for RasterBuffer in the worker thread.
- Moved RasterBuffer from ResourceProvider to its own file.
It's now the public interface used by raster tasks.
- Added Acquire/ReleaseSkCanvas() interface into
RasterBuffer.
- Bitmap conversion is now done in raster worker thread
through RasterBuffer interface.
BUG=None
Committed: https://crrev.com/5df4cc9eb3ef7e320328d07437e05d478f052a10
Cr-Commit-Position: refs/heads/master@{#291910}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Make RasterBuffer the public interface used by raster tasks. #
Total comments: 8
Patch Set 3 : Changed RasterBuffer interface. #
Total comments: 22
Patch Set 4 : Code refactoring. #
Total comments: 24
Patch Set 5 : More code refactoring and some interface changes. #
Total comments: 12
Patch Set 6 : Fix and cleanup. #
Total comments: 19
Patch Set 7 : Miscellaneous fixes #
Total comments: 6
Patch Set 8 : Fix. #Patch Set 9 : Rebased on top of master #Patch Set 10 : Fix. #Patch Set 11 : Fix for dbg unittest. #Patch Set 12 : Revert previous patch set. #Patch Set 13 : Rebase on master. #Patch Set 14 : Fix. #
Total comments: 3
Messages
Total messages: 69 (0 generated)
@reviewer: How does this patch look to you?
https://codereview.chromium.org/454843002/diff/1/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/454843002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:22: class RasterCanvas { I think this interface deserves it's own file. https://codereview.chromium.org/454843002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:24: virtual ~RasterCanvas(); can this be protected? https://codereview.chromium.org/454843002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:26: SkCanvas* GetSkCanvas() { return locked_canvas_; } Can we make this interface completely abstract? Only pure virtual functions? https://codereview.chromium.org/454843002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.h:449: class RasterBuffer : public RasterCanvas { Do we need both a RasterBuffer and a RasterCanvas interface? I'm not a fan of the existing use of inheritance for RasterBuffer code reuse. Maybe we can make RasterBuffer the public interface used by raster tasks and use a more appropriate mechanism for code reuse in RP.
https://codereview.chromium.org/454843002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.h:449: class RasterBuffer : public RasterCanvas { On 2014/08/11 11:46:21, reveman wrote: > Do we need both a RasterBuffer and a RasterCanvas interface? I'm not a fan of > the existing use of inheritance for RasterBuffer code reuse. Maybe we can make > RasterBuffer the public interface used by raster tasks and use a more > appropriate mechanism for code reuse in RP. I'll try to do that. Looks like RasterBuffer can easily be removed. I don't think lock/unlock functions need to be virtuals.
https://codereview.chromium.org/454843002/diff/1/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/454843002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:22: class RasterCanvas { On 2014/08/11 11:46:20, reveman wrote: > I think this interface deserves it's own file. Done. https://codereview.chromium.org/454843002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:24: virtual ~RasterCanvas(); On 2014/08/11 11:46:21, reveman wrote: > can this be protected? Done. https://codereview.chromium.org/454843002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:26: SkCanvas* GetSkCanvas() { return locked_canvas_; } On 2014/08/11 11:46:21, reveman wrote: > Can we make this interface completely abstract? Only pure virtual functions? Done.
https://codereview.chromium.org/454843002/diff/20001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/20001/cc/resources/raster_buff... cc/resources/raster_buffer.h:15: virtual void Flush() = 0; How about Acquire/ReleaseSkCanvas()? Acquire can allocate a temporary buffer and Release will perform format conversion if necessary. We could potentially add other functions that can be implemented without a temporary buffer. ie. Clear. https://codereview.chromium.org/454843002/diff/20001/cc/resources/raster_buff... cc/resources/raster_buffer.h:18: RasterBuffer(); you don't need this for fully abstract class. https://codereview.chromium.org/454843002/diff/20001/cc/resources/raster_buff... cc/resources/raster_buffer.h:19: virtual ~RasterBuffer(); you can add an empty implementation here and remove the .cc file: virtual ~RasterBuffer() {} https://codereview.chromium.org/454843002/diff/20001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/20001/cc/resources/resource_pr... cc/resources/resource_provider.h:321: void UnmapGpuRasterBuffer(ResourceId id); Map/Unmap doesn't make much sense in the GPU raster case. Can we rename and make the naming more consistent between different implementations of RasterBuffers? ie. {Acquire|Release}{Gpu|Image|Pixel}RasterBuffer
https://codereview.chromium.org/454843002/diff/20001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/20001/cc/resources/raster_buff... cc/resources/raster_buffer.h:15: virtual void Flush() = 0; On 2014/08/11 20:32:36, reveman wrote: > How about Acquire/ReleaseSkCanvas()? Acquire can allocate a temporary buffer and > Release will perform format conversion if necessary. We could potentially add > other functions that can be implemented without a temporary buffer. ie. Clear. Done. https://codereview.chromium.org/454843002/diff/20001/cc/resources/raster_buff... cc/resources/raster_buffer.h:18: RasterBuffer(); On 2014/08/11 20:32:36, reveman wrote: > you don't need this for fully abstract class. Done. https://codereview.chromium.org/454843002/diff/20001/cc/resources/raster_buff... cc/resources/raster_buffer.h:19: virtual ~RasterBuffer(); On 2014/08/11 20:32:36, reveman wrote: > you can add an empty implementation here and remove the .cc file: > virtual ~RasterBuffer() {} Done. https://codereview.chromium.org/454843002/diff/20001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/20001/cc/resources/resource_pr... cc/resources/resource_provider.h:321: void UnmapGpuRasterBuffer(ResourceId id); On 2014/08/11 20:32:36, reveman wrote: > Map/Unmap doesn't make much sense in the GPU raster case. Can we rename and make > the naming more consistent between different implementations of RasterBuffers? > ie. {Acquire|Release}{Gpu|Image|Pixel}RasterBuffer Done.
This looks good. I understand that some of the requests that are part of this review are outside the scope of what you originally set out to do with this patch but I think they are worthwhile cleanups that I would much appreciate if included in the same patch. FYI, this patch will also allow us to map zero-copy buffers on worker threads, which is great! https://codereview.chromium.org/454843002/diff/40001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/40001/cc/resources/raster_buff... cc/resources/raster_buffer.h:15: virtual void ReleaseSkCanvas() = 0; nit: maybe require the SkCanvas* to be passed to ReleaseSkCanvas() so that an implementation that is not interested in caching the canvas can just unref it there instead of keeping an internal ref to it. https://codereview.chromium.org/454843002/diff/40001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/454843002/diff/40001/cc/resources/rasterizer.... cc/resources/rasterizer.h:14: class SkCanvas; please remove this if no longer needed. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:340: bool UnmapPixelRasterBuffer(ResourceId id); Can we refactor this to be consistent with {Acquire|Release}{Gpu|Image}RasterBuffer? I'm thinking that Map/Unmap can be implemented as RasterBuffer::Acquire/ReleaseSkCanvas instead. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:458: bool UnlockForWrite(); Do we need these functions or can we just handle this as part of Acquire/ReleaseSkCanvas instead? https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:462: ResourceProvider* resource_provider() const { return resource_provider_; } do we really need these functions? https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:472: SkCanvas* locked_canvas_; If the canvas is passed to ReleaseSkCanvas? Can we just remove this a create a new canvas each time? https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:473: int canvas_save_count_; I'd like to remove this. If we create a new SkCanvas each time then it's not necessary. If we need to cache the canvas then I think it should be a requirement of the RasterBuffer client to have a balanced number of save/restore calls. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:487: bool UnlockForWrite(); I prefer if we just handle this as part of Acquire/ReleaseSkCanvas instead. It's really nasty to have a derived class call a base class function and that function is then calling a virtual function implemented by the derived class. I would much prefer if the final derived class implemented Acquire/ReleaseSkCanvas and we add whatever utility functions we need to share code between implementations but no other virtual functions.
https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:340: bool UnmapPixelRasterBuffer(ResourceId id); On 2014/08/12 14:31:26, reveman wrote: > Can we refactor this to be consistent with > {Acquire|Release}{Gpu|Image}RasterBuffer? I'm thinking that Map/Unmap can be > implemented as RasterBuffer::Acquire/ReleaseSkCanvas instead. I don't think Map/UnmapPixelBuffer can be called from a worker thread. It didn't work for me at least. Or did I do something wrong? https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:458: bool UnlockForWrite(); On 2014/08/12 14:31:27, reveman wrote: > Do we need these functions or can we just handle this as part of > Acquire/ReleaseSkCanvas instead? Can GpuRasterBuffer::CreateSurface() be called from a worker thread? https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:487: bool UnlockForWrite(); On 2014/08/12 14:31:27, reveman wrote: > I prefer if we just handle this as part of > Acquire/ReleaseSkCanvas instead. It's really nasty to have a derived class call > a base class function and that function is then calling a virtual function > implemented by the derived class. > > I would much prefer if the final derived class implemented > Acquire/ReleaseSkCanvas and we add whatever utility functions we need to share > code between implementations but no other virtual functions. I think Lock/UnlockForWrite need to be called from compositor thread. Or am I wrong? We can make them virtual and implement in the final derived class and remove Map/UnmapBuffer virtuals.
https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:340: bool UnmapPixelRasterBuffer(ResourceId id); On 2014/08/12 15:24:01, auygun wrote: > On 2014/08/12 14:31:26, reveman wrote: > > Can we refactor this to be consistent with > > {Acquire|Release}{Gpu|Image}RasterBuffer? I'm thinking that Map/Unmap can be > > implemented as RasterBuffer::Acquire/ReleaseSkCanvas instead. > > I don't think Map/UnmapPixelBuffer can be called from a worker thread. It didn't > work for me at least. Or did I do something wrong? Right. That doesn't work at the moment but we like to change that in the future. At least for ImageRasterBuffer. How about having AcquirePixelRasterBuffer map the buffer for now and BeginSetPixels/ReleasePixelRasterBuffer implicitly unmap it? That would match how we want this to work in the future. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:458: bool UnlockForWrite(); On 2014/08/12 15:24:01, auygun wrote: > On 2014/08/12 14:31:27, reveman wrote: > > Do we need these functions or can we just handle this as part of > > Acquire/ReleaseSkCanvas instead? > > Can GpuRasterBuffer::CreateSurface() be called from a worker thread? Good point. I'm not sure that will work so let's not change that as part of this patch. Can we create the surface as the buffer is created? https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:487: bool UnlockForWrite(); On 2014/08/12 15:24:01, auygun wrote: > On 2014/08/12 14:31:27, reveman wrote: > > I prefer if we just handle this as part of > > Acquire/ReleaseSkCanvas instead. It's really nasty to have a derived class > call > > a base class function and that function is then calling a virtual function > > implemented by the derived class. > > > > I would much prefer if the final derived class implemented > > Acquire/ReleaseSkCanvas and we add whatever utility functions we need to share > > code between implementations but no other virtual functions. > > I think Lock/UnlockForWrite need to be called from compositor thread. Or am I > wrong? > We can make them virtual and implement in the final derived class and remove > Map/UnmapBuffer virtuals. I prefer if we remove the BitmapRasterBuffer class and instead add some explicit functions to the ImageRasterBuffer and PixelRasterBuffer classes if needed. The current inheritance is not needed for code reuse and is just confusing imo.
https://codereview.chromium.org/454843002/diff/40001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/40001/cc/resources/raster_buff... cc/resources/raster_buffer.h:15: virtual void ReleaseSkCanvas() = 0; On 2014/08/12 14:31:26, reveman wrote: > nit: maybe require the SkCanvas* to be passed to ReleaseSkCanvas() so that an > implementation that is not interested in caching the canvas can just unref it > there instead of keeping an internal ref to it. Acknowledged. https://codereview.chromium.org/454843002/diff/40001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/454843002/diff/40001/cc/resources/rasterizer.... cc/resources/rasterizer.h:14: class SkCanvas; On 2014/08/12 14:31:26, reveman wrote: > please remove this if no longer needed. Acknowledged. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:340: bool UnmapPixelRasterBuffer(ResourceId id); On 2014/08/12 15:54:28, reveman wrote: > On 2014/08/12 15:24:01, auygun wrote: > > On 2014/08/12 14:31:26, reveman wrote: > > > Can we refactor this to be consistent with > > > {Acquire|Release}{Gpu|Image}RasterBuffer? I'm thinking that Map/Unmap can be > > > implemented as RasterBuffer::Acquire/ReleaseSkCanvas instead. > > > > I don't think Map/UnmapPixelBuffer can be called from a worker thread. It > didn't > > work for me at least. Or did I do something wrong? > > Right. That doesn't work at the moment but we like to change that in the future. > At least for ImageRasterBuffer. > > How about having AcquirePixelRasterBuffer map the buffer for now and > BeginSetPixels/ReleasePixelRasterBuffer implicitly unmap it? That would match > how we want this to work in the future. This involves some more change. PixelBufferRasterWorkerPool needs to know if the content was changed before calling BeginSetPixel. I'll see what I can do. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:458: bool UnlockForWrite(); On 2014/08/12 15:54:28, reveman wrote: > On 2014/08/12 15:24:01, auygun wrote: > > On 2014/08/12 14:31:27, reveman wrote: > > > Do we need these functions or can we just handle this as part of > > > Acquire/ReleaseSkCanvas instead? > > > > Can GpuRasterBuffer::CreateSurface() be called from a worker thread? > > Good point. I'm not sure that will work so let's not change that as part of this > patch. Can we create the surface as the buffer is created? We can move GpuRasterBuffer::CreateSurface() to ResourceProvider and create the surface and pass it to the buffer as it's created. Is that what you mean? https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:462: ResourceProvider* resource_provider() const { return resource_provider_; } On 2014/08/12 14:31:27, reveman wrote: > do we really need these functions? Acknowledged. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:472: SkCanvas* locked_canvas_; On 2014/08/12 14:31:26, reveman wrote: > If the canvas is passed to ReleaseSkCanvas? Can we just remove this a create a > new canvas each time? Acknowledged. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:473: int canvas_save_count_; On 2014/08/12 14:31:26, reveman wrote: > I'd like to remove this. If we create a new SkCanvas each time then it's not > necessary. If we need to cache the canvas then I think it should be a > requirement of the RasterBuffer client to have a balanced number of save/restore > calls. Acknowledged. https://codereview.chromium.org/454843002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:487: bool UnlockForWrite(); On 2014/08/12 15:54:28, reveman wrote: > On 2014/08/12 15:24:01, auygun wrote: > > On 2014/08/12 14:31:27, reveman wrote: > > > I prefer if we just handle this as part of > > > Acquire/ReleaseSkCanvas instead. It's really nasty to have a derived class > > call > > > a base class function and that function is then calling a virtual function > > > implemented by the derived class. > > > > > > I would much prefer if the final derived class implemented > > > Acquire/ReleaseSkCanvas and we add whatever utility functions we need to > share > > > code between implementations but no other virtual functions. > > > > I think Lock/UnlockForWrite need to be called from compositor thread. Or am I > > wrong? > > We can make them virtual and implement in the final derived class and remove > > Map/UnmapBuffer virtuals. > > I prefer if we remove the BitmapRasterBuffer class and instead add some explicit > functions to the ImageRasterBuffer and PixelRasterBuffer classes if needed. The > current inheritance is not needed for code reuse and is just confusing imo. Acknowledged.
"git cl upload" doesn't want to send another patch set for some reason. It wants to create a new review. Is it ok if I create a new review?
git cl issue 454843002
I've uploaded a new patch set. But it doesn't look like a squashed version of my branch. I hope it's ok.
https://codereview.chromium.org/454843002/diff/60001/cc/resources/image_copy_... File cc/resources/image_copy_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/image_copy_... cc/resources/image_copy_raster_worker_pool.cc:194: resource_provider_->ReleaseImageRasterBuffer(resource->id()); Let's keep having this return true when contents might have changed. https://codereview.chromium.org/454843002/diff/60001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:688: if (!raster_task->content_has_changed()) { Let's keep things simple and remove all this conditional "contents have changed" code from here. This is currently here to avoid an upload when analysis determined the tile to be solid. We can handle that in BeginSetPixels instead and avoid an upload at that level. This will make this code much cleaner without having a real impact on performance. https://codereview.chromium.org/454843002/diff/60001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/raster_buff... cc/resources/raster_buffer.h:16: virtual skia::RefPtr<SkCanvas> AcquireSkCanvas() = 0; Do we need to return a skia::RefPtr? It's weird that Acquire returns a ref but Release doesn't pass it back. https://codereview.chromium.org/454843002/diff/60001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/rasterizer.... cc/resources/rasterizer.h:78: bool content_has_changed() const { return content_has_changed_; } I don't think we need to add this. See my other comments. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:459: skia::RefPtr<SkSurface> ResourceProvider::GpuRasterBuffer::CreateSurface() { Is it still worth having this function? Looks like it might clean things up a bit by moving this code into the ctor.. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:528: raster_bitmap_generation_id_ = raster_bitmap_.getGenerationID(); Let's not bother with the generation Id anymore. That was only useful when returning a SkCanvas on the compositor thread. Now we can simply assume that the buffer has been modified if Acquire/ReleaseSkCanvas has been called. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:541: raster_bitmap_generation_id_ != raster_bitmap_.getGenerationID(); Remove generation id related code. It can be assumed that the buffer was generated if Acquire/ReleaseSkCanvas has been called. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:609: raster_bitmap_generation_id_ != raster_bitmap_.getGenerationID(); Same here. Remove generation id related code. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.h:475: void UnlockForWrite(); Map/Unmap? I think that would better explain what these functions do. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.h:499: void UnlockForWrite(); Map/Unmap?
https://codereview.chromium.org/454843002/diff/60001/cc/resources/image_copy_... File cc/resources/image_copy_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/image_copy_... cc/resources/image_copy_raster_worker_pool.cc:194: resource_provider_->ReleaseImageRasterBuffer(resource->id()); On 2014/08/13 19:19:49, reveman wrote: > Let's keep having this return true when contents might have changed. Done. https://codereview.chromium.org/454843002/diff/60001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:688: if (!raster_task->content_has_changed()) { On 2014/08/13 19:19:49, reveman wrote: > Let's keep things simple and remove all this conditional "contents have changed" > code from here. This is currently here to avoid an upload when analysis > determined the tile to be solid. We can handle that in BeginSetPixels instead > and avoid an upload at that level. This will make this code much cleaner without > having a real impact on performance. Unittests assume that BeginSetPixels always uploads. Also PixelBufferRasterWorkerPool still needs to know if any uploads has been performed since last flush. So I'm adding TryBeginSetPixels which calls BeginSetPixels if raster bitmap was modified. https://codereview.chromium.org/454843002/diff/60001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/raster_buff... cc/resources/raster_buffer.h:16: virtual skia::RefPtr<SkCanvas> AcquireSkCanvas() = 0; On 2014/08/13 19:19:49, reveman wrote: > Do we need to return a skia::RefPtr? It's weird that Acquire returns a ref but > Release doesn't pass it back. Isn't it better to pass around the skia::RefPtr instead of the raw pointer? Let's have Release pass it back. https://codereview.chromium.org/454843002/diff/60001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/rasterizer.... cc/resources/rasterizer.h:78: bool content_has_changed() const { return content_has_changed_; } On 2014/08/13 19:19:49, reveman wrote: > I don't think we need to add this. See my other comments. Done. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:459: skia::RefPtr<SkSurface> ResourceProvider::GpuRasterBuffer::CreateSurface() { On 2014/08/13 19:19:50, reveman wrote: > Is it still worth having this function? Looks like it might clean things up a > bit by moving this code into the ctor.. Done. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:528: raster_bitmap_generation_id_ = raster_bitmap_.getGenerationID(); On 2014/08/13 19:19:49, reveman wrote: > Let's not bother with the generation Id anymore. That was only useful when > returning a SkCanvas on the compositor thread. Now we can simply assume that the > buffer has been modified if Acquire/ReleaseSkCanvas has been called. Done. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:541: raster_bitmap_generation_id_ != raster_bitmap_.getGenerationID(); On 2014/08/13 19:19:50, reveman wrote: > Remove generation id related code. It can be assumed that the buffer was > generated if Acquire/ReleaseSkCanvas has been called. Done. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.cc:609: raster_bitmap_generation_id_ != raster_bitmap_.getGenerationID(); On 2014/08/13 19:19:49, reveman wrote: > Same here. Remove generation id related code. Done. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.h:475: void UnlockForWrite(); On 2014/08/13 19:19:50, reveman wrote: > Map/Unmap? I think that would better explain what these functions do. Done. https://codereview.chromium.org/454843002/diff/60001/cc/resources/resource_pr... cc/resources/resource_provider.h:499: void UnlockForWrite(); On 2014/08/13 19:19:50, reveman wrote: > Map/Unmap? Done.
https://codereview.chromium.org/454843002/diff/60001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:688: if (!raster_task->content_has_changed()) { On 2014/08/14 10:35:42, auygun wrote: > On 2014/08/13 19:19:49, reveman wrote: > > Let's keep things simple and remove all this conditional "contents have > changed" > > code from here. This is currently here to avoid an upload when analysis > > determined the tile to be solid. We can handle that in BeginSetPixels instead > > and avoid an upload at that level. This will make this code much cleaner > without > > having a real impact on performance. > > Unittests assume that BeginSetPixels always uploads. Also > PixelBufferRasterWorkerPool still needs to know if any uploads has been > performed since last flush. So I'm adding TryBeginSetPixels which calls > BeginSetPixels if raster bitmap was modified. Can we instead fix the unit tests and have PixelBufferRasterWorkerPool assume that all BeginSetPixels calls result in an upload? I think that would be much cleaner. There's already the case that BeginSetPixels might not actually perform an upload if the context is lost. https://codereview.chromium.org/454843002/diff/60001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/raster_buff... cc/resources/raster_buffer.h:16: virtual skia::RefPtr<SkCanvas> AcquireSkCanvas() = 0; On 2014/08/14 10:35:42, auygun wrote: > On 2014/08/13 19:19:49, reveman wrote: > > Do we need to return a skia::RefPtr? It's weird that Acquire returns a ref but > > Release doesn't pass it back. > > Isn't it better to pass around the skia::RefPtr instead of the raw pointer? > Let's have Release pass it back. Use skia::RefPtr if you need to pass ownership. Raw pointer is preferred if that's not the case. Looks like you need to pass ownership in this case though so skia::RefPtr is then correct. https://codereview.chromium.org/454843002/diff/80001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/raster_buff... cc/resources/raster_buffer.h:17: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) = 0; const ref? That makes it seem like ReleaseSkCanvas is not going to modify it. You're passing ownership so that's clearly not the case. https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:503: resource_provider_->UnmapImage(resource_); what if MapImage failed? it seems like it would be incorrect to call UnmapImage in that case. https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:564: resource_provider_->UnmapPixelBuffer(resource_); Same here. What if MapPixelBuffer failed? https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.h:463: const bool use_distance_field_text_; do you still need this member?
https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:503: resource_provider_->UnmapImage(resource_); On 2014/08/14 12:26:36, reveman wrote: > what if MapImage failed? it seems like it would be incorrect to call UnmapImage > in that case. I've changed this so that it doesn't call UnmapImage if mapped_buffer_ is NULL (which means MapBuffer failed). And then unittest failed: [FAIL] ResourceProviderTests/ResourceProviderTest.CopyResource_GLTexture/0: ../../cc/resources/resource_provider_unittest.cc:3143: Failure Actual function call count doesn't match EXPECT_CALL(*context, unmapImageCHROMIUM(kImageId))... Expected: to be called once Actual: never called - unsatisfied and active [FAIL] ResourceProviderTests/ResourceProviderTest.Image_GLTexture/0: ../../cc/resources/resource_provider_unittest.cc:2997: Failure Actual function call count doesn't match EXPECT_CALL(*context, unmapImageCHROMIUM(kImageId))... Expected: to be called once Actual: never called - unsatisfied and active ../../cc/resources/resource_provider_unittest.cc:3027: Failure Actual function call count doesn't match EXPECT_CALL(*context, unmapImageCHROMIUM(kImageId))... Expected: to be called once Actual: never called - unsatisfied and active It fails because of the following line in raster_worker_pool_unittest.cc context3d->set_times_map_image_chromium_succeeds(0); So mapImageCHROMIUM returns NULL. I don't understand what the intent is here. How to fix this?
On 2014/08/14 15:38:40, auygun wrote: > https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... > File cc/resources/resource_provider.cc (right): > > https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... > cc/resources/resource_provider.cc:503: > resource_provider_->UnmapImage(resource_); > On 2014/08/14 12:26:36, reveman wrote: > > what if MapImage failed? it seems like it would be incorrect to call > UnmapImage > > in that case. > > I've changed this so that it doesn't call UnmapImage if mapped_buffer_ is NULL > (which means MapBuffer failed). And then unittest failed: > > [FAIL] ResourceProviderTests/ResourceProviderTest.CopyResource_GLTexture/0: > ../../cc/resources/resource_provider_unittest.cc:3143: Failure > Actual function call count doesn't match EXPECT_CALL(*context, > unmapImageCHROMIUM(kImageId))... > Expected: to be called once > Actual: never called - unsatisfied and active > > [FAIL] ResourceProviderTests/ResourceProviderTest.Image_GLTexture/0: > ../../cc/resources/resource_provider_unittest.cc:2997: Failure > Actual function call count doesn't match EXPECT_CALL(*context, > unmapImageCHROMIUM(kImageId))... > Expected: to be called once > Actual: never called - unsatisfied and active > ../../cc/resources/resource_provider_unittest.cc:3027: Failure > Actual function call count doesn't match EXPECT_CALL(*context, > unmapImageCHROMIUM(kImageId))... > Expected: to be called once > Actual: never called - unsatisfied and active > > It fails because of the following line in raster_worker_pool_unittest.cc > > context3d->set_times_map_image_chromium_succeeds(0); > > So mapImageCHROMIUM returns NULL. I don't understand what the intent is here. > How to fix this? I was wrong. It's not that line. But I don't know why mapImageCHROMIUM returns NULL.
https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:503: resource_provider_->UnmapImage(resource_); On 2014/08/14 15:38:40, auygun wrote: > On 2014/08/14 12:26:36, reveman wrote: > > what if MapImage failed? it seems like it would be incorrect to call > UnmapImage > > in that case. > > I've changed this so that it doesn't call UnmapImage if mapped_buffer_ is NULL > (which means MapBuffer failed). And then unittest failed: > > [FAIL] ResourceProviderTests/ResourceProviderTest.CopyResource_GLTexture/0: > ../../cc/resources/resource_provider_unittest.cc:3143: Failure > Actual function call count doesn't match EXPECT_CALL(*context, > unmapImageCHROMIUM(kImageId))... > Expected: to be called once > Actual: never called - unsatisfied and active > > [FAIL] ResourceProviderTests/ResourceProviderTest.Image_GLTexture/0: > ../../cc/resources/resource_provider_unittest.cc:2997: Failure > Actual function call count doesn't match EXPECT_CALL(*context, > unmapImageCHROMIUM(kImageId))... > Expected: to be called once > Actual: never called - unsatisfied and active > ../../cc/resources/resource_provider_unittest.cc:3027: Failure > Actual function call count doesn't match EXPECT_CALL(*context, > unmapImageCHROMIUM(kImageId))... > Expected: to be called once > Actual: never called - unsatisfied and active > > It fails because of the following line in raster_worker_pool_unittest.cc > > context3d->set_times_map_image_chromium_succeeds(0); > > So mapImageCHROMIUM returns NULL. I don't understand what the intent is here. > How to fix this? I think the expectation is wrong. If mapImage fails then we should not expect unmapImage to be called. Just remove the EXPECT_CALL from the unit test.
https://codereview.chromium.org/454843002/diff/60001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:688: if (!raster_task->content_has_changed()) { On 2014/08/14 12:26:36, reveman wrote: > On 2014/08/14 10:35:42, auygun wrote: > > On 2014/08/13 19:19:49, reveman wrote: > > > Let's keep things simple and remove all this conditional "contents have > > changed" > > > code from here. This is currently here to avoid an upload when analysis > > > determined the tile to be solid. We can handle that in BeginSetPixels > instead > > > and avoid an upload at that level. This will make this code much cleaner > > without > > > having a real impact on performance. > > > > Unittests assume that BeginSetPixels always uploads. Also > > PixelBufferRasterWorkerPool still needs to know if any uploads has been > > performed since last flush. So I'm adding TryBeginSetPixels which calls > > BeginSetPixels if raster bitmap was modified. > > Can we instead fix the unit tests and have PixelBufferRasterWorkerPool assume > that all BeginSetPixels calls result in an upload? I think that would be much > cleaner. There's already the case that BeginSetPixels might not actually perform > an upload if the context is lost. Done. https://codereview.chromium.org/454843002/diff/60001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/60001/cc/resources/raster_buff... cc/resources/raster_buffer.h:16: virtual skia::RefPtr<SkCanvas> AcquireSkCanvas() = 0; On 2014/08/14 12:26:36, reveman wrote: > On 2014/08/14 10:35:42, auygun wrote: > > On 2014/08/13 19:19:49, reveman wrote: > > > Do we need to return a skia::RefPtr? It's weird that Acquire returns a ref > but > > > Release doesn't pass it back. > > > > Isn't it better to pass around the skia::RefPtr instead of the raw pointer? > > Let's have Release pass it back. > > Use skia::RefPtr if you need to pass ownership. Raw pointer is preferred if > that's not the case. Looks like you need to pass ownership in this case though > so skia::RefPtr is then correct. Acknowledged. https://codereview.chromium.org/454843002/diff/80001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/raster_buff... cc/resources/raster_buffer.h:17: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) = 0; On 2014/08/14 12:26:36, reveman wrote: > const ref? That makes it seem like ReleaseSkCanvas is not going to modify it. > You're passing ownership so that's clearly not the case. According to the documentation in skia/ext/refptr.h this is how to pass the ownership. // When passing around a ref-counted pointer to methods outside of Skia, always // pass around the skia::RefPtr instead of the raw pointer. An example method // that takes a SkShader* parameter and saves the SkShader* in the class. // void AMethodThatSavesAShader(const skia::RefPtr<SkShader>& shader) { // member_refptr_ = shader; // } // skia::RefPtr<SkShader> member_refptr_; https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:503: resource_provider_->UnmapImage(resource_); On 2014/08/14 16:14:38, reveman wrote: > On 2014/08/14 15:38:40, auygun wrote: > > On 2014/08/14 12:26:36, reveman wrote: > > > what if MapImage failed? it seems like it would be incorrect to call > > UnmapImage > > > in that case. > > > > I've changed this so that it doesn't call UnmapImage if mapped_buffer_ is NULL > > (which means MapBuffer failed). And then unittest failed: > > > > [FAIL] ResourceProviderTests/ResourceProviderTest.CopyResource_GLTexture/0: > > ../../cc/resources/resource_provider_unittest.cc:3143: Failure > > Actual function call count doesn't match EXPECT_CALL(*context, > > unmapImageCHROMIUM(kImageId))... > > Expected: to be called once > > Actual: never called - unsatisfied and active > > > > [FAIL] ResourceProviderTests/ResourceProviderTest.Image_GLTexture/0: > > ../../cc/resources/resource_provider_unittest.cc:2997: Failure > > Actual function call count doesn't match EXPECT_CALL(*context, > > unmapImageCHROMIUM(kImageId))... > > Expected: to be called once > > Actual: never called - unsatisfied and active > > ../../cc/resources/resource_provider_unittest.cc:3027: Failure > > Actual function call count doesn't match EXPECT_CALL(*context, > > unmapImageCHROMIUM(kImageId))... > > Expected: to be called once > > Actual: never called - unsatisfied and active > > > > It fails because of the following line in raster_worker_pool_unittest.cc > > > > context3d->set_times_map_image_chromium_succeeds(0); > > > > So mapImageCHROMIUM returns NULL. I don't understand what the intent is here. > > How to fix this? > > I think the expectation is wrong. If mapImage fails then we should not expect > unmapImage to be called. Just remove the EXPECT_CALL from the unit test. Done. https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.cc:564: resource_provider_->UnmapPixelBuffer(resource_); On 2014/08/14 12:26:36, reveman wrote: > Same here. What if MapPixelBuffer failed? Done. https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/resource_pr... cc/resources/resource_provider.h:463: const bool use_distance_field_text_; On 2014/08/14 12:26:37, reveman wrote: > do you still need this member? Done.
https://codereview.chromium.org/454843002/diff/80001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/raster_buff... cc/resources/raster_buffer.h:17: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) = 0; On 2014/08/15 09:20:40, auygun wrote: > On 2014/08/14 12:26:36, reveman wrote: > > const ref? That makes it seem like ReleaseSkCanvas is not going to modify it. > > You're passing ownership so that's clearly not the case. > > According to the documentation in skia/ext/refptr.h this is how to pass the > ownership. > > // When passing around a ref-counted pointer to methods outside of Skia, always > // pass around the skia::RefPtr instead of the raw pointer. An example method > // that takes a SkShader* parameter and saves the SkShader* in the class. > // void AMethodThatSavesAShader(const skia::RefPtr<SkShader>& shader) { > // member_refptr_ = shader; > // } > // skia::RefPtr<SkShader> member_refptr_; OK, we typically don't use const ref when passing ownership of a ref counted type in chromium but if that's what skis does and as this is a skia type, we should probably do that. Current patch looks good in this regard. Thanks for pointing out that this is common practice in skia code. https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (left): https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:720: state.required_for_activation; We still need this code in case the task is no longer in |raster_tasks_|. https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:685: if (!raster_task->HasFinishedRunning()) { Should we release the raster buffer in this case? https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:696: raster_task->DidComplete(); These functions need to be called even if the conditional above is not true. https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.cc:1933: if (resource->pixel_raster_buffer->UnmapBuffer()) { You can probably remove the comment above if you use a temporary variable instead of checking the return value of unmap directly. https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.h:456: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) OVERRIDE; Why are these functions protected? https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.h:477: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) OVERRIDE; Why are these functions protected? https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.h:501: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) OVERRIDE; Why are these functions protected?
https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (left): https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:720: state.required_for_activation; On 2014/08/15 11:54:31, reveman wrote: > We still need this code in case the task is no longer in |raster_tasks_|. Now the task is pushed into raster_tasks_with_pending_upload_ and later processed in CheckForCompletedUploads() even when BeginSetPixel() doesn't result in an upload. Are you sure we still need this code? https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:685: if (!raster_task->HasFinishedRunning()) { On 2014/08/15 11:54:31, reveman wrote: > Should we release the raster buffer in this case? Buffer is relased in RasterTaskImpl::CompleteOnOriginThread(). It's either called here or later in CheckForCompletedUploads(). https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:696: raster_task->DidComplete(); On 2014/08/15 11:54:31, reveman wrote: > These functions need to be called even if the conditional above is not true. Aren't they called later in CheckForCompletedUploads() as the task gets pushed to raster_tasks_with_pending_upload_ right after BeginSetPixels() call?
this looks great. address some of the last remaining comments and I think this is ready to land. https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (left): https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:720: state.required_for_activation; On 2014/08/15 12:24:20, auygun wrote: > On 2014/08/15 11:54:31, reveman wrote: > > We still need this code in case the task is no longer in |raster_tasks_|. > > Now the task is pushed into raster_tasks_with_pending_upload_ and later > processed in CheckForCompletedUploads() even when BeginSetPixel() doesn't result > in an upload. Are you sure we still need this code? It seemed a bit awkward to attempt an upload when the raster task was cancelled but I think you're right, it's better to always call BeginSetPixels in this case. You can can keep the code as is. https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:685: if (!raster_task->HasFinishedRunning()) { On 2014/08/15 12:24:19, auygun wrote: > On 2014/08/15 11:54:31, reveman wrote: > > Should we release the raster buffer in this case? > > Buffer is relased in RasterTaskImpl::CompleteOnOriginThread(). It's either > called here or later in CheckForCompletedUploads(). Acknowledged. https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:696: raster_task->DidComplete(); On 2014/08/15 12:24:20, auygun wrote: > On 2014/08/15 11:54:31, reveman wrote: > > These functions need to be called even if the conditional above is not true. > > Aren't they called later in CheckForCompletedUploads() as the task gets pushed > to raster_tasks_with_pending_upload_ right after BeginSetPixels() call? Acknowledged. https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.cc:1933: if (resource->pixel_raster_buffer->UnmapBuffer()) { btw, should we early out here instead? resource->pending_set_pixels should not be set to true unless the buffer was modified while mapped, right?
https://codereview.chromium.org/454843002/diff/80001/cc/resources/raster_buff... File cc/resources/raster_buffer.h (right): https://codereview.chromium.org/454843002/diff/80001/cc/resources/raster_buff... cc/resources/raster_buffer.h:17: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) = 0; On 2014/08/15 11:54:31, reveman wrote: > On 2014/08/15 09:20:40, auygun wrote: > > On 2014/08/14 12:26:36, reveman wrote: > > > const ref? That makes it seem like ReleaseSkCanvas is not going to modify > it. > > > You're passing ownership so that's clearly not the case. > > > > According to the documentation in skia/ext/refptr.h this is how to pass the > > ownership. > > > > // When passing around a ref-counted pointer to methods outside of Skia, > always > > // pass around the skia::RefPtr instead of the raw pointer. An example method > > // that takes a SkShader* parameter and saves the SkShader* in the class. > > // void AMethodThatSavesAShader(const skia::RefPtr<SkShader>& shader) { > > // member_refptr_ = shader; > > // } > > // skia::RefPtr<SkShader> member_refptr_; > > OK, we typically don't use const ref when passing ownership of a ref counted > type in chromium but if that's what skis does and as this is a skia type, we > should probably do that. Current patch looks good in this regard. Thanks for > pointing out that this is common practice in skia code. Acknowledged. https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (left): https://codereview.chromium.org/454843002/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:720: state.required_for_activation; On 2014/08/15 13:59:34, reveman wrote: > On 2014/08/15 12:24:20, auygun wrote: > > On 2014/08/15 11:54:31, reveman wrote: > > > We still need this code in case the task is no longer in |raster_tasks_|. > > > > Now the task is pushed into raster_tasks_with_pending_upload_ and later > > processed in CheckForCompletedUploads() even when BeginSetPixel() doesn't > result > > in an upload. Are you sure we still need this code? > > It seemed a bit awkward to attempt an upload when the raster task was cancelled > but I think you're right, it's better to always call BeginSetPixels in this > case. You can can keep the code as is. Ok. I think it's better to not to attempt an upload here. I'm changing this. https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.cc:1933: if (resource->pixel_raster_buffer->UnmapBuffer()) { On 2014/08/15 13:59:34, reveman wrote: > btw, should we early out here instead? resource->pending_set_pixels should not > be set to true unless the buffer was modified while mapped, right? Right. resource->pending_set_pixels should not be set to true in this case. https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.h:456: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) OVERRIDE; On 2014/08/15 11:54:31, reveman wrote: > Why are these functions protected? Done. https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.h:477: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) OVERRIDE; On 2014/08/15 11:54:31, reveman wrote: > Why are these functions protected? Done. https://codereview.chromium.org/454843002/diff/100001/cc/resources/resource_p... cc/resources/resource_provider.h:501: virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) OVERRIDE; On 2014/08/15 11:54:31, reveman wrote: > Why are these functions protected? Done.
https://codereview.chromium.org/454843002/diff/120001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:493: stride_ = 0; do you need to reset raster_bitmap_changed_ here? https://codereview.chromium.org/454843002/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:556: stride_ = 0; do you need to reset raster_bitmap_changed_ here? https://codereview.chromium.org/454843002/diff/120001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/454843002/diff/120001/cc/resources/tile_manag... cc/resources/tile_manager.cc:134: DCHECK(canvas); I think this currently needs to be allowed to fail in context lost situations (gpu process crashed so mapped_buffer_=NULL). It would be nice if we could return a dummy canvas in these situations so the code here doesn't have to be aware of this special case.
https://codereview.chromium.org/454843002/diff/120001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/454843002/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:493: stride_ = 0; On 2014/08/16 08:36:18, reveman wrote: > do you need to reset raster_bitmap_changed_ here? Done. https://codereview.chromium.org/454843002/diff/120001/cc/resources/resource_p... cc/resources/resource_provider.cc:556: stride_ = 0; On 2014/08/16 08:36:18, reveman wrote: > do you need to reset raster_bitmap_changed_ here? Done. https://codereview.chromium.org/454843002/diff/120001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/454843002/diff/120001/cc/resources/tile_manag... cc/resources/tile_manager.cc:134: DCHECK(canvas); On 2014/08/16 08:36:18, reveman wrote: > I think this currently needs to be allowed to fail in context lost situations > (gpu process crashed so mapped_buffer_=NULL). It would be nice if we could > return a dummy canvas in these situations so the code here doesn't have to be > aware of this special case. I'm adding a null-check here.
lgtm
The CQ bit was checked by auygun@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/auygun@opera.com/454843002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43022) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/48201) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43025) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I've just done a rebase on top of master. Does it need another review now?
The CQ bit was checked by auygun@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/auygun@opera.com/454843002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...) android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by auygun@opera.com
The CQ bit was checked by auygun@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/auygun@opera.com/454843002/180001
The CQ bit was unchecked by auygun@opera.com
cc_unittests in debug hit the DCHECK below. It's called from RasterTaskImpl::Analyze() [FATAL:picture.cc(390)] Check failed: raster_thread_checker_.CalledOnValidThread(). I don't know what exactly going on. I noticed that GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So AcquireSkCanvas() needs to be called before Analyze() to check if there is a valid canvas. In this case we also cannot assume that bitmap was changed when AcquireSkCanvas() is called so I'm putting the bitmap generation id check back into the RasterBuffer implementations.
The CQ bit was checked by auygun@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/auygun@opera.com/454843002/200001
The CQ bit was unchecked by reveman@chromium.org
On 2014/08/19 10:27:00, auygun wrote: > cc_unittests in debug hit the DCHECK below. It's called from > RasterTaskImpl::Analyze() > > [FATAL:picture.cc(390)] Check failed: > raster_thread_checker_.CalledOnValidThread(). > > I don't know what exactly going on. I noticed that > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So AcquireSkCanvas() > needs to be called before Analyze() to check if there is a valid canvas. In this > case we also cannot assume that bitmap was changed when AcquireSkCanvas() is > called so I'm putting the bitmap generation id check back into the RasterBuffer > implementations. cq- as I'd like to understand why we need the old generation id code before we land this. Why are we suddenly trying to raster on a different thread? Why does AcquireSkCanvas() need to be called before Analyze()?
On 2014/08/19 11:34:58, reveman wrote: > On 2014/08/19 10:27:00, auygun wrote: > > cc_unittests in debug hit the DCHECK below. It's called from > > RasterTaskImpl::Analyze() > > > > [FATAL:picture.cc(390)] Check failed: > > raster_thread_checker_.CalledOnValidThread(). > > > > I don't know what exactly going on. I noticed that > > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So AcquireSkCanvas() > > needs to be called before Analyze() to check if there is a valid canvas. In > this > > case we also cannot assume that bitmap was changed when AcquireSkCanvas() is > > called so I'm putting the bitmap generation id check back into the > RasterBuffer > > implementations. > > cq- as I'd like to understand why we need the old generation id code before we > land this. Why are we suddenly trying to raster on a different thread? Why does > AcquireSkCanvas() need to be called before Analyze()? Apparently GpuRasterWorkerPool runs raster tasks on main thread. Here is the callstack for the failing DCHECK: cc::Picture::Raster(SkCanvas*, SkDrawPictureCallback*, cc::Region const&, float) at /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture.cc:391 cc::PicturePileImpl::RasterCommon(SkCanvas*, SkDrawPictureCallback*, gfx::Rect const&, float, cc::RenderingStatsInstrumentation*, bool) at /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:312 cc::PicturePileImpl::RasterForAnalysis(skia::AnalysisCanvas*, gfx::Rect const&, float, cc::RenderingStatsInstrumentation*) at /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:93 (discriminator 3) cc::PicturePileImpl::AnalyzeInRect(gfx::Rect const&, float, cc::PicturePileImpl::Analysis*, cc::RenderingStatsInstrumentation*) at /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:381 Analyze at /home/attila/projects/upstream/src/out/Debug/../../cc/resources/tile_manager.cc:117 cc::TaskGraphRunner::RunTaskWithLockAcquired() at /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:422 cc::TaskGraphRunner::RunUntilIdle() at /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:377 cc::GpuRasterWorkerPool::RunTasksOnOriginThread() at /home/attila/projects/upstream/src/out/Debug/../../cc/resources/gpu_raster_worker_pool.cc:194 (discriminator 2) base::internal::RunnableAdapter<void (cc::GpuRasterWorkerPool::*)()>::Run(cc::GpuRasterWorkerPool*) at /home/attila/projects/upstream/src/out/Debug/../../base/bind_internal.h:134 base::Callback<void ()>::Run() const at /home/attila/projects/upstream/src/out/Debug/../../base/callback.h:401 base::MessageLoop::RunTask(base::PendingTask const&) at /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:436 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:445 base::MessageLoop::DoWork() at /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:552 base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_pump_default.cc:32 base::MessageLoop::RunHandler() at /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:408 base::RunLoop::Run() at /home/attila/projects/upstream/src/out/Debug/../../base/run_loop.cc:49 base::MessageLoop::Run() at /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:301 base::Thread::ThreadMain() at /home/attila/projects/upstream/src/out/Debug/../../base/threading/thread.cc:228 ThreadFunc at /home/attila/projects/upstream/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 I don't think this DCHECK in cc_unittest is related to canvas being NULL, however, it can be avoided by null-checking canvas before Analyse(). Also in the base code the NULL check for canvas was done before Analyze(). Looks like that's why debug cc_unittest was not failing before this patch.
On 2014/08/19 12:14:12, auygun wrote: > On 2014/08/19 11:34:58, reveman wrote: > > On 2014/08/19 10:27:00, auygun wrote: > > > cc_unittests in debug hit the DCHECK below. It's called from > > > RasterTaskImpl::Analyze() > > > > > > [FATAL:picture.cc(390)] Check failed: > > > raster_thread_checker_.CalledOnValidThread(). > > > > > > I don't know what exactly going on. I noticed that > > > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So > AcquireSkCanvas() > > > needs to be called before Analyze() to check if there is a valid canvas. In > > this > > > case we also cannot assume that bitmap was changed when AcquireSkCanvas() is > > > called so I'm putting the bitmap generation id check back into the > > RasterBuffer > > > implementations. > > > > cq- as I'd like to understand why we need the old generation id code before we > > land this. Why are we suddenly trying to raster on a different thread? Why > does > > AcquireSkCanvas() need to be called before Analyze()? > > Apparently GpuRasterWorkerPool runs raster tasks on main thread. Here is the > callstack for the failing DCHECK: > > cc::Picture::Raster(SkCanvas*, SkDrawPictureCallback*, cc::Region const&, float) > at > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture.cc:391 > cc::PicturePileImpl::RasterCommon(SkCanvas*, SkDrawPictureCallback*, gfx::Rect > const&, float, cc::RenderingStatsInstrumentation*, bool) at > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:312 > cc::PicturePileImpl::RasterForAnalysis(skia::AnalysisCanvas*, gfx::Rect const&, > float, cc::RenderingStatsInstrumentation*) at > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:93 > (discriminator 3) > cc::PicturePileImpl::AnalyzeInRect(gfx::Rect const&, float, > cc::PicturePileImpl::Analysis*, cc::RenderingStatsInstrumentation*) at > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:381 > Analyze at > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/tile_manager.cc:117 > cc::TaskGraphRunner::RunTaskWithLockAcquired() at > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:422 > cc::TaskGraphRunner::RunUntilIdle() at > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:377 > cc::GpuRasterWorkerPool::RunTasksOnOriginThread() at > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/gpu_raster_worker_pool.cc:194 > (discriminator 2) > base::internal::RunnableAdapter<void > (cc::GpuRasterWorkerPool::*)()>::Run(cc::GpuRasterWorkerPool*) at > /home/attila/projects/upstream/src/out/Debug/../../base/bind_internal.h:134 > base::Callback<void ()>::Run() const at > /home/attila/projects/upstream/src/out/Debug/../../base/callback.h:401 > base::MessageLoop::RunTask(base::PendingTask const&) at > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:436 > base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:445 > base::MessageLoop::DoWork() at > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:552 > base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_pump_default.cc:32 > base::MessageLoop::RunHandler() at > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:408 > base::RunLoop::Run() at > /home/attila/projects/upstream/src/out/Debug/../../base/run_loop.cc:49 > base::MessageLoop::Run() at > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:301 > base::Thread::ThreadMain() at > /home/attila/projects/upstream/src/out/Debug/../../base/threading/thread.cc:228 > ThreadFunc at > /home/attila/projects/upstream/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 > > I don't think this DCHECK in cc_unittest is related to canvas being NULL, > however, it can be avoided by null-checking canvas before Analyse(). > Also in the base code the NULL check for canvas was done before Analyze(). Looks > like that's why debug cc_unittest was not failing before this patch. Ok, we need to get to the bottom of this before we can land this patch as this might be a real issue. Pictures can be used for raster on either the compositor thread or a raster thread. But not both. That DCHECK will fail if the picture is used on both and that would be a bug that we need to fix. What's the specific unit test that is failing? Btw, it's also a bit suspicious that we're doing analysis for gpu raster, I don't think we should be anymore.
On 2014/08/19 12:27:11, reveman wrote: > On 2014/08/19 12:14:12, auygun wrote: > > On 2014/08/19 11:34:58, reveman wrote: > > > On 2014/08/19 10:27:00, auygun wrote: > > > > cc_unittests in debug hit the DCHECK below. It's called from > > > > RasterTaskImpl::Analyze() > > > > > > > > [FATAL:picture.cc(390)] Check failed: > > > > raster_thread_checker_.CalledOnValidThread(). > > > > > > > > I don't know what exactly going on. I noticed that > > > > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So > > AcquireSkCanvas() > > > > needs to be called before Analyze() to check if there is a valid canvas. > In > > > this > > > > case we also cannot assume that bitmap was changed when AcquireSkCanvas() > is > > > > called so I'm putting the bitmap generation id check back into the > > > RasterBuffer > > > > implementations. > > > > > > cq- as I'd like to understand why we need the old generation id code before > we > > > land this. Why are we suddenly trying to raster on a different thread? Why > > does > > > AcquireSkCanvas() need to be called before Analyze()? > > > > Apparently GpuRasterWorkerPool runs raster tasks on main thread. Here is the > > callstack for the failing DCHECK: > > > > cc::Picture::Raster(SkCanvas*, SkDrawPictureCallback*, cc::Region const&, > float) > > at > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture.cc:391 > > cc::PicturePileImpl::RasterCommon(SkCanvas*, SkDrawPictureCallback*, gfx::Rect > > const&, float, cc::RenderingStatsInstrumentation*, bool) at > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:312 > > cc::PicturePileImpl::RasterForAnalysis(skia::AnalysisCanvas*, gfx::Rect > const&, > > float, cc::RenderingStatsInstrumentation*) at > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:93 > > (discriminator 3) > > cc::PicturePileImpl::AnalyzeInRect(gfx::Rect const&, float, > > cc::PicturePileImpl::Analysis*, cc::RenderingStatsInstrumentation*) at > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:381 > > Analyze at > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/tile_manager.cc:117 > > cc::TaskGraphRunner::RunTaskWithLockAcquired() at > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:422 > > cc::TaskGraphRunner::RunUntilIdle() at > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:377 > > cc::GpuRasterWorkerPool::RunTasksOnOriginThread() at > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/gpu_raster_worker_pool.cc:194 > > (discriminator 2) > > base::internal::RunnableAdapter<void > > (cc::GpuRasterWorkerPool::*)()>::Run(cc::GpuRasterWorkerPool*) at > > /home/attila/projects/upstream/src/out/Debug/../../base/bind_internal.h:134 > > base::Callback<void ()>::Run() const at > > /home/attila/projects/upstream/src/out/Debug/../../base/callback.h:401 > > base::MessageLoop::RunTask(base::PendingTask const&) at > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:436 > > base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:445 > > base::MessageLoop::DoWork() at > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:552 > > base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_pump_default.cc:32 > > base::MessageLoop::RunHandler() at > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:408 > > base::RunLoop::Run() at > > /home/attila/projects/upstream/src/out/Debug/../../base/run_loop.cc:49 > > base::MessageLoop::Run() at > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:301 > > base::Thread::ThreadMain() at > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/thread.cc:228 > > ThreadFunc at > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 > > > > I don't think this DCHECK in cc_unittest is related to canvas being NULL, > > however, it can be avoided by null-checking canvas before Analyse(). > > Also in the base code the NULL check for canvas was done before Analyze(). > Looks > > like that's why debug cc_unittest was not failing before this patch. > > Ok, we need to get to the bottom of this before we can land this patch as this > might be a real issue. Pictures can be used for raster on either the compositor > thread or a raster thread. But not both. That DCHECK will fail if the picture is > used on both and that would be a bug that we need to fix. What's the specific > unit test that is failing? Btw, it's also a bit suspicious that we're doing > analysis for gpu raster, I don't think we should be anymore. This is the failing unit test: LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_DirectRenderer_MainThreadPaint
On 2014/08/19 12:40:10, auygun wrote: > On 2014/08/19 12:27:11, reveman wrote: > > On 2014/08/19 12:14:12, auygun wrote: > > > On 2014/08/19 11:34:58, reveman wrote: > > > > On 2014/08/19 10:27:00, auygun wrote: > > > > > cc_unittests in debug hit the DCHECK below. It's called from > > > > > RasterTaskImpl::Analyze() > > > > > > > > > > [FATAL:picture.cc(390)] Check failed: > > > > > raster_thread_checker_.CalledOnValidThread(). > > > > > > > > > > I don't know what exactly going on. I noticed that > > > > > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So > > > AcquireSkCanvas() > > > > > needs to be called before Analyze() to check if there is a valid canvas. > > In > > > > this > > > > > case we also cannot assume that bitmap was changed when > AcquireSkCanvas() > > is > > > > > called so I'm putting the bitmap generation id check back into the > > > > RasterBuffer > > > > > implementations. > > > > > > > > cq- as I'd like to understand why we need the old generation id code > before > > we > > > > land this. Why are we suddenly trying to raster on a different thread? Why > > > does > > > > AcquireSkCanvas() need to be called before Analyze()? > > > > > > Apparently GpuRasterWorkerPool runs raster tasks on main thread. Here is the > > > callstack for the failing DCHECK: > > > > > > cc::Picture::Raster(SkCanvas*, SkDrawPictureCallback*, cc::Region const&, > > float) > > > at > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture.cc:391 > > > cc::PicturePileImpl::RasterCommon(SkCanvas*, SkDrawPictureCallback*, > gfx::Rect > > > const&, float, cc::RenderingStatsInstrumentation*, bool) at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:312 > > > cc::PicturePileImpl::RasterForAnalysis(skia::AnalysisCanvas*, gfx::Rect > > const&, > > > float, cc::RenderingStatsInstrumentation*) at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:93 > > > (discriminator 3) > > > cc::PicturePileImpl::AnalyzeInRect(gfx::Rect const&, float, > > > cc::PicturePileImpl::Analysis*, cc::RenderingStatsInstrumentation*) at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:381 > > > Analyze at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/tile_manager.cc:117 > > > cc::TaskGraphRunner::RunTaskWithLockAcquired() at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:422 > > > cc::TaskGraphRunner::RunUntilIdle() at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:377 > > > cc::GpuRasterWorkerPool::RunTasksOnOriginThread() at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/gpu_raster_worker_pool.cc:194 > > > (discriminator 2) > > > base::internal::RunnableAdapter<void > > > (cc::GpuRasterWorkerPool::*)()>::Run(cc::GpuRasterWorkerPool*) at > > > /home/attila/projects/upstream/src/out/Debug/../../base/bind_internal.h:134 > > > base::Callback<void ()>::Run() const at > > > /home/attila/projects/upstream/src/out/Debug/../../base/callback.h:401 > > > base::MessageLoop::RunTask(base::PendingTask const&) at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:436 > > > base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:445 > > > base::MessageLoop::DoWork() at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:552 > > > base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_pump_default.cc:32 > > > base::MessageLoop::RunHandler() at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:408 > > > base::RunLoop::Run() at > > > /home/attila/projects/upstream/src/out/Debug/../../base/run_loop.cc:49 > > > base::MessageLoop::Run() at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:301 > > > base::Thread::ThreadMain() at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/thread.cc:228 > > > ThreadFunc at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 > > > > > > I don't think this DCHECK in cc_unittest is related to canvas being NULL, > > > however, it can be avoided by null-checking canvas before Analyse(). > > > Also in the base code the NULL check for canvas was done before Analyze(). > > Looks > > > like that's why debug cc_unittest was not failing before this patch. > > > > Ok, we need to get to the bottom of this before we can land this patch as this > > might be a real issue. Pictures can be used for raster on either the > compositor > > thread or a raster thread. But not both. That DCHECK will fail if the picture > is > > used on both and that would be a bug that we need to fix. What's the specific > > unit test that is failing? Btw, it's also a bit suspicious that we're doing > > analysis for gpu raster, I don't think we should be anymore. > > This is the failing unit test: > LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_DirectRenderer_MainThreadPaint I don't think we should be running any gpu rasterization tests with main thread painting as that's not a supported configuration. Are we running .RunMultiThread_DirectRenderer_ImplSidePaint too and that's passing?
+alok any idea why we have main-thread-paint test for gpu rasterization?
On 2014/08/19 12:56:25, reveman wrote: > On 2014/08/19 12:40:10, auygun wrote: > > On 2014/08/19 12:27:11, reveman wrote: > > > On 2014/08/19 12:14:12, auygun wrote: > > > > On 2014/08/19 11:34:58, reveman wrote: > > > > > On 2014/08/19 10:27:00, auygun wrote: > > > > > > cc_unittests in debug hit the DCHECK below. It's called from > > > > > > RasterTaskImpl::Analyze() > > > > > > > > > > > > [FATAL:picture.cc(390)] Check failed: > > > > > > raster_thread_checker_.CalledOnValidThread(). > > > > > > > > > > > > I don't know what exactly going on. I noticed that > > > > > > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So > > > > AcquireSkCanvas() > > > > > > needs to be called before Analyze() to check if there is a valid > canvas. > > > In > > > > > this > > > > > > case we also cannot assume that bitmap was changed when > > AcquireSkCanvas() > > > is > > > > > > called so I'm putting the bitmap generation id check back into the > > > > > RasterBuffer > > > > > > implementations. > > > > > > > > > > cq- as I'd like to understand why we need the old generation id code > > before > > > we > > > > > land this. Why are we suddenly trying to raster on a different thread? > Why > > > > does > > > > > AcquireSkCanvas() need to be called before Analyze()? > > > > > > > > Apparently GpuRasterWorkerPool runs raster tasks on main thread. Here is > the > > > > callstack for the failing DCHECK: > > > > > > > > cc::Picture::Raster(SkCanvas*, SkDrawPictureCallback*, cc::Region const&, > > > float) > > > > at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture.cc:391 > > > > cc::PicturePileImpl::RasterCommon(SkCanvas*, SkDrawPictureCallback*, > > gfx::Rect > > > > const&, float, cc::RenderingStatsInstrumentation*, bool) at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:312 > > > > cc::PicturePileImpl::RasterForAnalysis(skia::AnalysisCanvas*, gfx::Rect > > > const&, > > > > float, cc::RenderingStatsInstrumentation*) at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:93 > > > > (discriminator 3) > > > > cc::PicturePileImpl::AnalyzeInRect(gfx::Rect const&, float, > > > > cc::PicturePileImpl::Analysis*, cc::RenderingStatsInstrumentation*) at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:381 > > > > Analyze at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/tile_manager.cc:117 > > > > cc::TaskGraphRunner::RunTaskWithLockAcquired() at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:422 > > > > cc::TaskGraphRunner::RunUntilIdle() at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:377 > > > > cc::GpuRasterWorkerPool::RunTasksOnOriginThread() at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/gpu_raster_worker_pool.cc:194 > > > > (discriminator 2) > > > > base::internal::RunnableAdapter<void > > > > (cc::GpuRasterWorkerPool::*)()>::Run(cc::GpuRasterWorkerPool*) at > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/bind_internal.h:134 > > > > base::Callback<void ()>::Run() const at > > > > /home/attila/projects/upstream/src/out/Debug/../../base/callback.h:401 > > > > base::MessageLoop::RunTask(base::PendingTask const&) at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:436 > > > > base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:445 > > > > base::MessageLoop::DoWork() at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:552 > > > > base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_pump_default.cc:32 > > > > base::MessageLoop::RunHandler() at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:408 > > > > base::RunLoop::Run() at > > > > /home/attila/projects/upstream/src/out/Debug/../../base/run_loop.cc:49 > > > > base::MessageLoop::Run() at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:301 > > > > base::Thread::ThreadMain() at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/thread.cc:228 > > > > ThreadFunc at > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 > > > > > > > > I don't think this DCHECK in cc_unittest is related to canvas being NULL, > > > > however, it can be avoided by null-checking canvas before Analyse(). > > > > Also in the base code the NULL check for canvas was done before Analyze(). > > > Looks > > > > like that's why debug cc_unittest was not failing before this patch. > > > > > > Ok, we need to get to the bottom of this before we can land this patch as > this > > > might be a real issue. Pictures can be used for raster on either the > > compositor > > > thread or a raster thread. But not both. That DCHECK will fail if the > picture > > is > > > used on both and that would be a bug that we need to fix. What's the > specific > > > unit test that is failing? Btw, it's also a bit suspicious that we're doing > > > analysis for gpu raster, I don't think we should be anymore. > > > > This is the failing unit test: > > > LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_DirectRenderer_MainThreadPaint > > I don't think we should be running any gpu rasterization tests with main thread > painting as that's not a supported configuration. Are we running > .RunMultiThread_DirectRenderer_ImplSidePaint too and that's passing? It runs .RunMultiThread_DirectRenderer_ImplSidePaint and that one is also hitting the same DCHECK with the same callback. .RunMultiThread_DelegatingRenderer_MainThreadPaint and .RunMultiThread_DelegatingRenderer_ImplSidePaint are passing.
On 2014/08/19 13:37:31, auygun wrote: > On 2014/08/19 12:56:25, reveman wrote: > > On 2014/08/19 12:40:10, auygun wrote: > > > On 2014/08/19 12:27:11, reveman wrote: > > > > On 2014/08/19 12:14:12, auygun wrote: > > > > > On 2014/08/19 11:34:58, reveman wrote: > > > > > > On 2014/08/19 10:27:00, auygun wrote: > > > > > > > cc_unittests in debug hit the DCHECK below. It's called from > > > > > > > RasterTaskImpl::Analyze() > > > > > > > > > > > > > > [FATAL:picture.cc(390)] Check failed: > > > > > > > raster_thread_checker_.CalledOnValidThread(). > > > > > > > > > > > > > > I don't know what exactly going on. I noticed that > > > > > > > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So > > > > > AcquireSkCanvas() > > > > > > > needs to be called before Analyze() to check if there is a valid > > canvas. > > > > In > > > > > > this > > > > > > > case we also cannot assume that bitmap was changed when > > > AcquireSkCanvas() > > > > is > > > > > > > called so I'm putting the bitmap generation id check back into the > > > > > > RasterBuffer > > > > > > > implementations. > > > > > > > > > > > > cq- as I'd like to understand why we need the old generation id code > > > before > > > > we > > > > > > land this. Why are we suddenly trying to raster on a different thread? > > Why > > > > > does > > > > > > AcquireSkCanvas() need to be called before Analyze()? > > > > > > > > > > Apparently GpuRasterWorkerPool runs raster tasks on main thread. Here is > > the > > > > > callstack for the failing DCHECK: > > > > > > > > > > cc::Picture::Raster(SkCanvas*, SkDrawPictureCallback*, cc::Region > const&, > > > > float) > > > > > at > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture.cc:391 > > > > > cc::PicturePileImpl::RasterCommon(SkCanvas*, SkDrawPictureCallback*, > > > gfx::Rect > > > > > const&, float, cc::RenderingStatsInstrumentation*, bool) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:312 > > > > > cc::PicturePileImpl::RasterForAnalysis(skia::AnalysisCanvas*, gfx::Rect > > > > const&, > > > > > float, cc::RenderingStatsInstrumentation*) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:93 > > > > > (discriminator 3) > > > > > cc::PicturePileImpl::AnalyzeInRect(gfx::Rect const&, float, > > > > > cc::PicturePileImpl::Analysis*, cc::RenderingStatsInstrumentation*) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:381 > > > > > Analyze at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/tile_manager.cc:117 > > > > > cc::TaskGraphRunner::RunTaskWithLockAcquired() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:422 > > > > > cc::TaskGraphRunner::RunUntilIdle() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:377 > > > > > cc::GpuRasterWorkerPool::RunTasksOnOriginThread() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/gpu_raster_worker_pool.cc:194 > > > > > (discriminator 2) > > > > > base::internal::RunnableAdapter<void > > > > > (cc::GpuRasterWorkerPool::*)()>::Run(cc::GpuRasterWorkerPool*) at > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/bind_internal.h:134 > > > > > base::Callback<void ()>::Run() const at > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/callback.h:401 > > > > > base::MessageLoop::RunTask(base::PendingTask const&) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:436 > > > > > base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:445 > > > > > base::MessageLoop::DoWork() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:552 > > > > > base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_pump_default.cc:32 > > > > > base::MessageLoop::RunHandler() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:408 > > > > > base::RunLoop::Run() at > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/run_loop.cc:49 > > > > > base::MessageLoop::Run() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:301 > > > > > base::Thread::ThreadMain() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/thread.cc:228 > > > > > ThreadFunc at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 > > > > > > > > > > I don't think this DCHECK in cc_unittest is related to canvas being > NULL, > > > > > however, it can be avoided by null-checking canvas before Analyse(). > > > > > Also in the base code the NULL check for canvas was done before > Analyze(). > > > > Looks > > > > > like that's why debug cc_unittest was not failing before this patch. > > > > > > > > Ok, we need to get to the bottom of this before we can land this patch as > > this > > > > might be a real issue. Pictures can be used for raster on either the > > > compositor > > > > thread or a raster thread. But not both. That DCHECK will fail if the > > picture > > > is > > > > used on both and that would be a bug that we need to fix. What's the > > specific > > > > unit test that is failing? Btw, it's also a bit suspicious that we're > doing > > > > analysis for gpu raster, I don't think we should be anymore. > > > > > > This is the failing unit test: > > > > > > LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_DirectRenderer_MainThreadPaint > > > > I don't think we should be running any gpu rasterization tests with main > thread > > painting as that's not a supported configuration. Are we running > > .RunMultiThread_DirectRenderer_ImplSidePaint too and that's passing? > > It runs .RunMultiThread_DirectRenderer_ImplSidePaint and that one is also > hitting the same DCHECK with the same callback. > .RunMultiThread_DelegatingRenderer_MainThreadPaint and > .RunMultiThread_DelegatingRenderer_ImplSidePaint are passing. Are the memory limits set such that this triggers on-demand raster? That would explain why DelegatingRenderer works but DirectRenderer doesn't.
On 2014/08/19 13:37:31, auygun wrote: > On 2014/08/19 12:56:25, reveman wrote: > > On 2014/08/19 12:40:10, auygun wrote: > > > On 2014/08/19 12:27:11, reveman wrote: > > > > On 2014/08/19 12:14:12, auygun wrote: > > > > > On 2014/08/19 11:34:58, reveman wrote: > > > > > > On 2014/08/19 10:27:00, auygun wrote: > > > > > > > cc_unittests in debug hit the DCHECK below. It's called from > > > > > > > RasterTaskImpl::Analyze() > > > > > > > > > > > > > > [FATAL:picture.cc(390)] Check failed: > > > > > > > raster_thread_checker_.CalledOnValidThread(). > > > > > > > > > > > > > > I don't know what exactly going on. I noticed that > > > > > > > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So > > > > > AcquireSkCanvas() > > > > > > > needs to be called before Analyze() to check if there is a valid > > canvas. > > > > In > > > > > > this > > > > > > > case we also cannot assume that bitmap was changed when > > > AcquireSkCanvas() > > > > is > > > > > > > called so I'm putting the bitmap generation id check back into the > > > > > > RasterBuffer > > > > > > > implementations. > > > > > > > > > > > > cq- as I'd like to understand why we need the old generation id code > > > before > > > > we > > > > > > land this. Why are we suddenly trying to raster on a different thread? > > Why > > > > > does > > > > > > AcquireSkCanvas() need to be called before Analyze()? > > > > > > > > > > Apparently GpuRasterWorkerPool runs raster tasks on main thread. Here is > > the > > > > > callstack for the failing DCHECK: > > > > > > > > > > cc::Picture::Raster(SkCanvas*, SkDrawPictureCallback*, cc::Region > const&, > > > > float) > > > > > at > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture.cc:391 > > > > > cc::PicturePileImpl::RasterCommon(SkCanvas*, SkDrawPictureCallback*, > > > gfx::Rect > > > > > const&, float, cc::RenderingStatsInstrumentation*, bool) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:312 > > > > > cc::PicturePileImpl::RasterForAnalysis(skia::AnalysisCanvas*, gfx::Rect > > > > const&, > > > > > float, cc::RenderingStatsInstrumentation*) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:93 > > > > > (discriminator 3) > > > > > cc::PicturePileImpl::AnalyzeInRect(gfx::Rect const&, float, > > > > > cc::PicturePileImpl::Analysis*, cc::RenderingStatsInstrumentation*) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:381 > > > > > Analyze at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/tile_manager.cc:117 > > > > > cc::TaskGraphRunner::RunTaskWithLockAcquired() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:422 > > > > > cc::TaskGraphRunner::RunUntilIdle() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:377 > > > > > cc::GpuRasterWorkerPool::RunTasksOnOriginThread() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/gpu_raster_worker_pool.cc:194 > > > > > (discriminator 2) > > > > > base::internal::RunnableAdapter<void > > > > > (cc::GpuRasterWorkerPool::*)()>::Run(cc::GpuRasterWorkerPool*) at > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/bind_internal.h:134 > > > > > base::Callback<void ()>::Run() const at > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/callback.h:401 > > > > > base::MessageLoop::RunTask(base::PendingTask const&) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:436 > > > > > base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:445 > > > > > base::MessageLoop::DoWork() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:552 > > > > > base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_pump_default.cc:32 > > > > > base::MessageLoop::RunHandler() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:408 > > > > > base::RunLoop::Run() at > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/run_loop.cc:49 > > > > > base::MessageLoop::Run() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:301 > > > > > base::Thread::ThreadMain() at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/thread.cc:228 > > > > > ThreadFunc at > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 > > > > > > > > > > I don't think this DCHECK in cc_unittest is related to canvas being > NULL, > > > > > however, it can be avoided by null-checking canvas before Analyse(). > > > > > Also in the base code the NULL check for canvas was done before > Analyze(). > > > > Looks > > > > > like that's why debug cc_unittest was not failing before this patch. > > > > > > > > Ok, we need to get to the bottom of this before we can land this patch as > > this > > > > might be a real issue. Pictures can be used for raster on either the > > > compositor > > > > thread or a raster thread. But not both. That DCHECK will fail if the > > picture > > > is > > > > used on both and that would be a bug that we need to fix. What's the > > specific > > > > unit test that is failing? Btw, it's also a bit suspicious that we're > doing > > > > analysis for gpu raster, I don't think we should be anymore. > > > > > > This is the failing unit test: > > > > > > LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_DirectRenderer_MainThreadPaint > > > > I don't think we should be running any gpu rasterization tests with main > thread > > painting as that's not a supported configuration. Are we running > > .RunMultiThread_DirectRenderer_ImplSidePaint too and that's passing? > > It runs .RunMultiThread_DirectRenderer_ImplSidePaint and that one is also > hitting the same DCHECK with the same callback. > .RunMultiThread_DelegatingRenderer_MainThreadPaint and > .RunMultiThread_DelegatingRenderer_ImplSidePaint are passing. Are the memory limits set such that this triggers on-demand raster? That would explain why DelegatingRenderer works but DirectRenderer doesn't.
On 2014/08/19 14:41:10, reveman wrote: > On 2014/08/19 13:37:31, auygun wrote: > > On 2014/08/19 12:56:25, reveman wrote: > > > On 2014/08/19 12:40:10, auygun wrote: > > > > On 2014/08/19 12:27:11, reveman wrote: > > > > > On 2014/08/19 12:14:12, auygun wrote: > > > > > > On 2014/08/19 11:34:58, reveman wrote: > > > > > > > On 2014/08/19 10:27:00, auygun wrote: > > > > > > > > cc_unittests in debug hit the DCHECK below. It's called from > > > > > > > > RasterTaskImpl::Analyze() > > > > > > > > > > > > > > > > [FATAL:picture.cc(390)] Check failed: > > > > > > > > raster_thread_checker_.CalledOnValidThread(). > > > > > > > > > > > > > > > > I don't know what exactly going on. I noticed that > > > > > > > > GpuRasterBuffer::AcquireSkCanvas() returns a NULL canvas. So > > > > > > AcquireSkCanvas() > > > > > > > > needs to be called before Analyze() to check if there is a valid > > > canvas. > > > > > In > > > > > > > this > > > > > > > > case we also cannot assume that bitmap was changed when > > > > AcquireSkCanvas() > > > > > is > > > > > > > > called so I'm putting the bitmap generation id check back into the > > > > > > > RasterBuffer > > > > > > > > implementations. > > > > > > > > > > > > > > cq- as I'd like to understand why we need the old generation id code > > > > before > > > > > we > > > > > > > land this. Why are we suddenly trying to raster on a different > thread? > > > Why > > > > > > does > > > > > > > AcquireSkCanvas() need to be called before Analyze()? > > > > > > > > > > > > Apparently GpuRasterWorkerPool runs raster tasks on main thread. Here > is > > > the > > > > > > callstack for the failing DCHECK: > > > > > > > > > > > > cc::Picture::Raster(SkCanvas*, SkDrawPictureCallback*, cc::Region > > const&, > > > > > float) > > > > > > at > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture.cc:391 > > > > > > cc::PicturePileImpl::RasterCommon(SkCanvas*, SkDrawPictureCallback*, > > > > gfx::Rect > > > > > > const&, float, cc::RenderingStatsInstrumentation*, bool) at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:312 > > > > > > cc::PicturePileImpl::RasterForAnalysis(skia::AnalysisCanvas*, > gfx::Rect > > > > > const&, > > > > > > float, cc::RenderingStatsInstrumentation*) at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:93 > > > > > > (discriminator 3) > > > > > > cc::PicturePileImpl::AnalyzeInRect(gfx::Rect const&, float, > > > > > > cc::PicturePileImpl::Analysis*, cc::RenderingStatsInstrumentation*) at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/picture_pile_impl.cc:381 > > > > > > Analyze at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/tile_manager.cc:117 > > > > > > cc::TaskGraphRunner::RunTaskWithLockAcquired() at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:422 > > > > > > cc::TaskGraphRunner::RunUntilIdle() at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/task_graph_runner.cc:377 > > > > > > cc::GpuRasterWorkerPool::RunTasksOnOriginThread() at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../cc/resources/gpu_raster_worker_pool.cc:194 > > > > > > (discriminator 2) > > > > > > base::internal::RunnableAdapter<void > > > > > > (cc::GpuRasterWorkerPool::*)()>::Run(cc::GpuRasterWorkerPool*) at > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/bind_internal.h:134 > > > > > > base::Callback<void ()>::Run() const at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/callback.h:401 > > > > > > base::MessageLoop::RunTask(base::PendingTask const&) at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:436 > > > > > > base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:445 > > > > > > base::MessageLoop::DoWork() at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:552 > > > > > > base::MessagePumpDefault::Run(base::MessagePump::Delegate*) at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_pump_default.cc:32 > > > > > > base::MessageLoop::RunHandler() at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:408 > > > > > > base::RunLoop::Run() at > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/run_loop.cc:49 > > > > > > base::MessageLoop::Run() at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/message_loop/message_loop.cc:301 > > > > > > base::Thread::ThreadMain() at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/thread.cc:228 > > > > > > ThreadFunc at > > > > > > > > > > > > > > > > > > > > > /home/attila/projects/upstream/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 > > > > > > > > > > > > I don't think this DCHECK in cc_unittest is related to canvas being > > NULL, > > > > > > however, it can be avoided by null-checking canvas before Analyse(). > > > > > > Also in the base code the NULL check for canvas was done before > > Analyze(). > > > > > Looks > > > > > > like that's why debug cc_unittest was not failing before this patch. > > > > > > > > > > Ok, we need to get to the bottom of this before we can land this patch > as > > > this > > > > > might be a real issue. Pictures can be used for raster on either the > > > > compositor > > > > > thread or a raster thread. But not both. That DCHECK will fail if the > > > picture > > > > is > > > > > used on both and that would be a bug that we need to fix. What's the > > > specific > > > > > unit test that is failing? Btw, it's also a bit suspicious that we're > > doing > > > > > analysis for gpu raster, I don't think we should be anymore. > > > > > > > > This is the failing unit test: > > > > > > > > > > LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_DirectRenderer_MainThreadPaint > > > > > > I don't think we should be running any gpu rasterization tests with main > > thread > > > painting as that's not a supported configuration. Are we running > > > .RunMultiThread_DirectRenderer_ImplSidePaint too and that's passing? > > > > It runs .RunMultiThread_DirectRenderer_ImplSidePaint and that one is also > > hitting the same DCHECK with the same callback. > > .RunMultiThread_DelegatingRenderer_MainThreadPaint and > > .RunMultiThread_DelegatingRenderer_ImplSidePaint are passing. > > Are the memory limits set such that this triggers on-demand raster? That would > explain why DelegatingRenderer works but DirectRenderer doesn't. I don't see any memory limits set for those tests.
+boliu who maintains LayerTreeHostTestDeferredInitializeWithGpuRasterization
On 2014/08/19 16:24:21, Alok Priyadarshi wrote: > +boliu who maintains LayerTreeHostTestDeferredInitializeWithGpuRasterization Long thread. What should I look at?
On 2014/08/19 16:29:43, boliu wrote: > On 2014/08/19 16:24:21, Alok Priyadarshi wrote: > > +boliu who maintains LayerTreeHostTestDeferredInitializeWithGpuRasterization > > Long thread. What should I look at? Oh probably this: http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... Gpu raster used to happen on the compositor thread. Is it being moved to a raster thread in this CL? In that case, you need to fix LayerTreeHostImpl::RunOnDemandRasterTask to run the task on the same thread.
On 2014/08/19 16:41:57, boliu wrote: > On 2014/08/19 16:29:43, boliu wrote: > > On 2014/08/19 16:24:21, Alok Priyadarshi wrote: > > > +boliu who maintains LayerTreeHostTestDeferredInitializeWithGpuRasterization > > > > Long thread. What should I look at? > > Oh probably this: > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... > > Gpu raster used to happen on the compositor thread. Is it being moved to a > raster thread in this CL? In that case, you need to fix > LayerTreeHostImpl::RunOnDemandRasterTask to run the task on the same thread. Actually this CL doesn't change where a raster task runs. So Gpu raster still should be happening in compositor thread.
boliu: I think they needed help in figuring out why LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_DirectRenderer_MainThreadPaint is failing.
LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_DirectRenderer_MainThreadPaint is failing in debug by hitting DCHECK: [FATAL:picture.cc(390)] Check failed: raster_thread_checker_.CalledOnValidThread(). during analyze in raster task.
Unit test doesn't fail after rebasing on latest master. I've just reverted patch set 11. I'll do another upload for rebase.
The CQ bit was checked by auygun@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/auygun@opera.com/454843002/260001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #14 (260001) as c69c9e426481a89dd3aec9474f683c56108cd3a2
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/5df4cc9eb3ef7e320328d07437e05d478f052a10 Cr-Commit-Position: refs/heads/master@{#291910}
Message was sent while issue was closed.
https://codereview.chromium.org/454843002/diff/260001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/454843002/diff/260001/cc/resources/tile_manag... cc/resources/tile_manager.cc:128: canvas->save(); Sorry, I think I missed this while reviewing. Why did you add save()/restore() calls to this function? Are they necessary for all raster buffer implementations?
Message was sent while issue was closed.
https://codereview.chromium.org/454843002/diff/260001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/454843002/diff/260001/cc/resources/tile_manag... cc/resources/tile_manager.cc:128: canvas->save(); On 2014/09/10 21:12:22, reveman wrote: > Sorry, I think I missed this while reviewing. Why did you add save()/restore() > calls to this function? Are they necessary for all raster buffer > implementations? All tiles were black when using GPU raster buffer. Adding save/restore fixed it. But the pixel raster buffer works without save/restore. I thought it would be good to have save/restore regardless of which raster buffer is used so that ReleaseSkCanvas() can also use the returned canvas (i.e. marking tiles for debugging purposes).
Message was sent while issue was closed.
https://codereview.chromium.org/454843002/diff/260001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/454843002/diff/260001/cc/resources/tile_manag... cc/resources/tile_manager.cc:128: canvas->save(); On 2014/09/11 08:27:07, auygun wrote: > On 2014/09/10 21:12:22, reveman wrote: > > Sorry, I think I missed this while reviewing. Why did you add save()/restore() > > calls to this function? Are they necessary for all raster buffer > > implementations? > > All tiles were black when using GPU raster buffer. Adding save/restore fixed it. > But the pixel raster buffer works without save/restore. > I thought it would be good to have save/restore regardless of which raster > buffer is used so that ReleaseSkCanvas() can also use the returned canvas (i.e. > marking tiles for debugging purposes). OK, thanks! FYI, I might move this into the raster buffer as part of a large refactor I'm working on. |
