[chromium] Use CCTexture in CCPrioritizedTexture, remove TextureManager.h, and minor refactoring.
Created attachment 151753 [details] Patch
Comment on attachment 151753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151753&action=review yes! very cool. Although i was hoping TextureManager.h was being rm -f entirely :) We'll get there soon! > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:34 > + nit: extra line :) > Source/WebCore/platform/graphics/chromium/cc/CCTexture.h:39 > + : m_id(id), m_size(size), m_format(format) { } these should be split up over 4 lines. and the : should be 4-space indented.
Yeah this patch is actually pretty minor. But figured I'd upload it while it was small and self-contained. Looks like it might be possible to nuke the old manager from orbit now. I'll try to do that and add it to this patch!
Created attachment 151778 [details] whomp
Comment on attachment 151778 [details] whomp View in context: https://bugs.webkit.org/attachment.cgi?id=151778&action=review > Source/WebCore/platform/graphics/chromium/TextureAllocator.h:1 > +/* I added this as TextureAllocator.h rather than cc/CCTextureAllocator.h intentionally to be consistent with derived classes (TrackingTextureAllocator) and similar classes (TextureUploader etc.). I figure the names should be changed together if we decide to do that, and this has become pretty big already.
Comment on attachment 151778 [details] whomp View in context: https://bugs.webkit.org/attachment.cgi?id=151778&action=review > Source/WebKit/chromium/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You need to remove this and add something here. prepare-ChangeLog has gotten snarkier. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221 > + m_client->setMemoryAllocationLimitBytes(64 * 1024 * 1024); This is strange. You bake this literal in here and in CCLayerTreeHostImpl, but aren't both of them overridden by messages from the GPU memory manager? If we need to have some initial pre-message value, can you make it a setting or somesuch? Is there a better solution? >> Source/WebCore/platform/graphics/chromium/TextureAllocator.h:1 >> +/* > > I added this as TextureAllocator.h rather than cc/CCTextureAllocator.h intentionally to be consistent with derived classes (TrackingTextureAllocator) and similar classes (TextureUploader etc.). I figure the names should be changed together if we decide to do that, and this has become pretty big already. Sounds fine to me. There's plenty of renaming that'll happen in the future. > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.cpp:154 > + > + nit: unneeded whitespace
Comment on attachment 151778 [details] whomp View in context: https://bugs.webkit.org/attachment.cgi?id=151778&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221 >> + m_client->setMemoryAllocationLimitBytes(64 * 1024 * 1024); > > This is strange. You bake this literal in here and in CCLayerTreeHostImpl, but aren't both of them overridden by messages from the GPU memory manager? > > If we need to have some initial pre-message value, can you make it a setting or somesuch? Is there a better solution? Yeah this needs a better solution. The default was just to be safe but I'll try to understand exactly when we fall back on these defaults such that maybe they can be removed.
Comment on attachment 151778 [details] whomp View in context: https://bugs.webkit.org/attachment.cgi?id=151778&action=review >>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221 >>> + m_client->setMemoryAllocationLimitBytes(64 * 1024 * 1024); >> >> This is strange. You bake this literal in here and in CCLayerTreeHostImpl, but aren't both of them overridden by messages from the GPU memory manager? >> >> If we need to have some initial pre-message value, can you make it a setting or somesuch? Is there a better solution? > > Yeah this needs a better solution. The default was just to be safe but I'll try to understand exactly when we fall back on these defaults such that maybe they can be removed. AIUI its there just for layout tests. DRT's GPU process doesnt have a mem manager. I think that the CCLTHI can initialize its value to 0, since it always gets a value from here. And here we can just use anything to support DRT such as this value. Why did you choose 1024 * 1024 * 16 * 4channels?
Comment on attachment 151778 [details] whomp View in context: https://bugs.webkit.org/attachment.cgi?id=151778&action=review >>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221 >>>> + m_client->setMemoryAllocationLimitBytes(64 * 1024 * 1024); >>> >>> This is strange. You bake this literal in here and in CCLayerTreeHostImpl, but aren't both of them overridden by messages from the GPU memory manager? >>> >>> If we need to have some initial pre-message value, can you make it a setting or somesuch? Is there a better solution? >> >> Yeah this needs a better solution. The default was just to be safe but I'll try to understand exactly when we fall back on these defaults such that maybe they can be removed. > > AIUI its there just for layout tests. DRT's GPU process doesnt have a mem manager. I think that the CCLTHI can initialize its value to 0, since it always gets a value from here. And here we can just use anything to support DRT such as this value. Why did you choose 1024 * 1024 * 16 * 4channels? What if we set the callback but it doesn't arrive for a while? It seems like the limit of zero can propagate from here to CCLayerTreeHostImpl and into CCLayerTreeHost before the message arrives (unless we delay rendering for a non-zero limit somewhere that I'm not seeing), whereas with highLimitBytes as a default we would still have memory right away on initialization Regarding 64MB I just picked that as it in highLimitBytes(), although looking now that appears to only be for android.
Comment on attachment 151778 [details] whomp View in context: https://bugs.webkit.org/attachment.cgi?id=151778&action=review >>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221 >>>>> + m_client->setMemoryAllocationLimitBytes(64 * 1024 * 1024); >>>> >>>> This is strange. You bake this literal in here and in CCLayerTreeHostImpl, but aren't both of them overridden by messages from the GPU memory manager? >>>> >>>> If we need to have some initial pre-message value, can you make it a setting or somesuch? Is there a better solution? >>> >>> Yeah this needs a better solution. The default was just to be safe but I'll try to understand exactly when we fall back on these defaults such that maybe they can be removed. >> >> AIUI its there just for layout tests. DRT's GPU process doesnt have a mem manager. I think that the CCLTHI can initialize its value to 0, since it always gets a value from here. And here we can just use anything to support DRT such as this value. Why did you choose 1024 * 1024 * 16 * 4channels? > > What if we set the callback but it doesn't arrive for a while? It seems like the limit of zero can propagate from here to CCLayerTreeHostImpl and into CCLayerTreeHost before the message arrives (unless we delay rendering for a non-zero limit somewhere that I'm not seeing), whereas with highLimitBytes as a default we would still have memory right away on initialization > > Regarding 64MB I just picked that as it in highLimitBytes(), although looking now that appears to only be for android. What if I add the highLimitBytes in this file with a FIXME, and we always call m_client->setMemoryAllocationLimitBytes(highLimitBytes(viewportSize()) here before setting the callback. That will replicate the old behavior while we figure out the best solution for the initialization race?
Created attachment 151837 [details] add_fixme_remove_whitespace
Created attachment 151843 [details] add_fixme_remove_whitespace
Created attachment 151844 [details] remove_more_whitespace
Comment on attachment 151844 [details] remove_more_whitespace View in context: https://bugs.webkit.org/attachment.cgi?id=151844&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:126 > - , m_memoryAllocationLimitBytes(TextureManager::highLimitBytes(viewportSize())) > + , m_memoryAllocationLimitBytes(CCPrioritizedTextureManager::defaultMemoryAllocationLimit()) It's still a little weird to double-set the initial limit. Who should be responsible for that? Since LRC is the one that receives the GPU memory manager limits in the general case, maybe this line should initialize to zero, and then we get an initial limit after initializing LRC? > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:58 > + // FIXME: This 96MB default is a straggler from the old texture manager and > + // is just to give us a default memory allocation before we get a callback > + // from the GPU memory manager. We should probaby either: > + // - wait for the callback before rendering anything instead > + // - make a synchronous call to GPU mem-manager to get the default (it can > + // decide whether to block or just return this default itself). Can you file a crbug about getting initial limits from the memory manager and put a link here?
There were two reasons for the default value, both have been mentioned but just confirming: - DRT needs a non 0 default since we will never get an allocation ipc, but LRC already handles this directly (see the else case of the if (m_capabilities.usingGpuMemoryManager) block in LRC::initialize). - We set a non 0 default so as we can generate frames before first allocation message arrives. For the second point, I am not sure its important. This default only applies to the first time you switch to a new tab -- for subsequent tab switches we wait until we receive a non-0 memory allocation, so setting a default on init case seems needlessly different than the usual case. Also, I thought the default used to be 64M not 96?
Comment on attachment 151844 [details] remove_more_whitespace View in context: https://bugs.webkit.org/attachment.cgi?id=151844&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:126 >> + , m_memoryAllocationLimitBytes(CCPrioritizedTextureManager::defaultMemoryAllocationLimit()) > > It's still a little weird to double-set the initial limit. Who should be responsible for that? > > Since LRC is the one that receives the GPU memory manager limits in the general case, maybe this line should initialize to zero, and then we get an initial limit after initializing LRC? Unfortunately, removing either was breaking tests: - This call is not early enough for the CCLTHI, as the zero limit still makes it through to CCLTH::updateLayers - Removing this call breaks initializationWithoutGpuMemoryManagerExtensionSupportShouldDefaultToNonZeroAllocation as the mock doesn't set the limit in the constructor like CCLTHI. Would it be acceptable to add these details to the bug?
Created attachment 152031 [details] rebase_add_bug_link_use_64mb_default
(In reply to comment #16) > > Unfortunately, removing either was breaking tests: > - This call is not early enough for the CCLTHI, as the zero limit still makes it through to CCLTH::updateLayers > - Removing this call breaks initializationWithoutGpuMemoryManagerExtensionSupportShouldDefaultToNonZeroAllocation as the mock doesn't set the limit in the constructor like CCLTHI. > > Would it be acceptable to add these details to the bug? I would prefer to not have duplicate initialization. It just seems sloppy. Given what you say above, can you initialize the limit in the CCLTHI constructor and the mock and remove the LRC call? Or, find some other place common to all paths to do this initialization?
(In reply to comment #18) > (In reply to comment #16) > > > > Unfortunately, removing either was breaking tests: > > - This call is not early enough for the CCLTHI, as the zero limit still makes it through to CCLTH::updateLayers > > - Removing this call breaks initializationWithoutGpuMemoryManagerExtensionSupportShouldDefaultToNonZeroAllocation as the mock doesn't set the limit in the constructor like CCLTHI. > > > > Would it be acceptable to add these details to the bug? > > I would prefer to not have duplicate initialization. It just seems sloppy. > > Given what you say above, can you initialize the limit in the CCLTHI constructor and the mock and remove the LRC call? Or, find some other place common to all paths to do this initialization? Sounds good.
Created attachment 152040 [details] remove_double_init
Comment on attachment 152040 [details] remove_double_init R=me. Thanks for that extra cleanup.
Comment on attachment 152040 [details] remove_double_init Ready to go.
Comment on attachment 152040 [details] remove_double_init Clearing flags on attachment: 152040 Committed r122506: <http://trac.webkit.org/changeset/122506>
All reviewed patches have been landed. Closing bug.