Issue metadata
Sign in to add a comment
|
Regression: Browser gets crashed after entering on Sign in overlay.
Reported by
db...@etouch.net,
Nov 9 2017
|
||||||||||||||||||||||
Issue descriptionChrome Version: 64.0.3263.0 Revision fd1e89557fa78a467061f732a8473fd863a89b1a-refs/heads/master@{#515052} OS: Mac(10.12.6,10.13.2) What steps will reproduce the problem? (1) Launch chrome, open dev tools window and press Esc key to open Condole drawer. (2) Navigate to Sensors section, change drop down option from 'Touch' and reload the page. (3) Observe. Actual: Browser gets crashed. CrashId: Uploaded Crash Report ID 3aa3e808c3369b6c (Local Crash ID: d0430ffd-f52b-42e2-b11f-2a22445797a3) Expected: Browser should not get crash. This is a regression issue, broken in 'M64'will soon update the other info: Good Build: 64.0.3262.0 Bad Build: 64.0.3263.0 Note: issue is not seen on Windows and Linux OS.
,
Nov 9 2017
Stack Trace for the Crash ID provided: -------------------------------------- Thread 0 (id: 84878) CRASHED [EXC_BAD_ACCESS / KERN_INVALID_ADDRESS @ 0x000000a0 ] MAGIC SIGNATURE THREAD Stack Quality70%Show frame trust levels 0x000000010453915a (Google Chrome Framework -color_space.cc:168 ) gfx::ColorSpace::operator!=(gfx::ColorSpace const&) const 0x000000010499077c (Google Chrome Framework -resource_pool.cc:172 ) cc::ResourcePool::ReuseResource(gfx::Size const&, viz::ResourceFormat, gfx::ColorSpace const&) 0x0000000104990bab (Google Chrome Framework -resource_pool.cc:220 ) cc::ResourcePool::AcquireResource(gfx::Size const&, viz::ResourceFormat, gfx::ColorSpace const&) 0x0000000104e999ce (Google Chrome Framework -gl_renderer.cc:3328 ) viz::GLRenderer::CopyRenderPassDrawQuadToOverlayResource(viz::CALayerOverlay const*, cc::Resource**, gfx::RectF*) 0x0000000104e990e4 (Google Chrome Framework -gl_renderer.cc:3418 ) viz::GLRenderer::ScheduleRenderPassDrawQuad(viz::CALayerOverlay const*) 0x0000000104e97141 (Google Chrome Framework -gl_renderer.cc:3124 ) viz::GLRenderer::ScheduleCALayers() 0x0000000104e969f1 (Google Chrome Framework -gl_renderer.cc:2603 ) viz::GLRenderer::FinishDrawingFrame() 0x0000000104e82195 (Google Chrome Framework -direct_renderer.cc:365 ) viz::DirectRenderer::DrawFrame(std::__1::vector<std::__1::unique_ptr<viz::RenderPass, std::__1::default_delete<viz::RenderPass> >, std::__1::allocator<std::__1::unique_ptr<viz::RenderPass, std::__1::default_delete<viz::RenderPass> > > >*, float, gfx::Size const&) 0x0000000104e865dd (Google Chrome Framework -display.cc:349 ) viz::Display::DrawAndSwap() 0x0000000104e893b4 (Google Chrome Framework -display_scheduler.cc:202 ) viz::DisplayScheduler::DrawAndSwap() 0x0000000104e885ab (Google Chrome Framework -display_scheduler.cc:489 ) viz::DisplayScheduler::OnBeginFrameDeadline() 0x0000000104c18a45 (Google Chrome Framework -callback.h:65 ) ui::(anonymous namespace)::WrappedTask::Run() 0x0000000103b0985b (Google Chrome Framework -callback.h:65 ) base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) 0x0000000103b2eac3 (Google Chrome Framework -message_loop.cc:394 ) base::MessageLoop::RunTask(base::PendingTask*) 0x0000000103b2eff8 (Google Chrome Framework -message_loop.cc:406 ) base::MessageLoop::DoWork() 0x0000000103b30e29 (Google Chrome Framework -message_pump_mac.mm:452 ) base::MessagePumpCFRunLoopBase::RunWork() 0x0000000103b21ce9 (Google Chrome Framework + 0x01cface9 ) base::mac::CallWithEHFrame(void () block_pointer) 0x0000000103b3074e (Google Chrome Framework -message_pump_mac.mm:428 ) base::MessagePumpCFRunLoopBase::RunWorkSource(void*) 0x00007fff8a0e57e0 (CoreFoundation + 0x000aa7e0 ) 0x00007fff8a0c4f0b (CoreFoundation + 0x00089f0b ) 0x00007fff8a0c442e (CoreFoundation + 0x0008942e ) 0x00007fff8a0c3e27 (CoreFoundation + 0x00088e27 ) 0x00007fff980ba934 (HIToolbox + 0x00030934 ) RunCurrentEventLoopInMode 0x00007fff980ba76e (HIToolbox + 0x0003076e ) ReceiveNextEventCommon 0x00007fff980ba5ae (HIToolbox + 0x000305ae ) _BlockUntilNextEventMatchingListInModeWithFilter 0x00007fff93e98df5 (AppKit + 0x00048df5 ) _DPSNextEvent 0x00007fff93e98225 (AppKit + 0x00048225 ) -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] 0x0000000103772c5f (Google Chrome Framework -chrome_browser_application_mac.mm:175 ) __71-[BrowserCrApplication nextEventMatchingMask:untilDate:inMode:dequeue:]_block_invoke 0x0000000103b21ce9 (Google Chrome Framework + 0x01cface9 ) base::mac::CallWithEHFrame(void () block_pointer) 0x0000000103772ba3 (Google Chrome Framework -chrome_browser_application_mac.mm:174 ) -[BrowserCrApplication nextEventMatchingMask:untilDate:inMode:dequeue:] 0x00007fff93e8cd7f (AppKit + 0x0003cd7f ) -[NSApplication run] 0x0000000103b316e6 (Google Chrome Framework -message_pump_mac.mm:804 ) base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) 0x0000000103b3026d (Google Chrome Framework -message_pump_mac.mm:179 ) base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) 0x0000000103b533c3 (Google Chrome Framework -run_loop.cc:114 ) <name omitted> 0x00000001037784c7 (Google Chrome Framework -chrome_browser_main.cc:1919 ) ChromeBrowserMainParts::MainMessageLoopRun(int*) 0x00000001025843f3 (Google Chrome Framework -browser_main_loop.cc:1206 ) content::BrowserMainLoop::RunMainMessageLoopParts() 0x00000001025874b1 (Google Chrome Framework -browser_main_runner.cc:140 ) content::BrowserMainRunnerImpl::Run() 0x000000010258092b (Google Chrome Framework -browser_main.cc:46 ) content::BrowserMain(content::MainFunctionParams const&) 0x000000010372d9ee (Google Chrome Framework -content_main_runner.cc:705 ) content::ContentMainRunnerImpl::Run() 0x0000000104f97dfa (Google Chrome Framework -main.cc:456 ) service_manager::Main(service_manager::MainParams const&) 0x000000010372cfa3 (Google Chrome Framework -content_main.cc:19 ) content::ContentMain(content::ContentMainParams const&) 0x0000000101e2ae6e (Google Chrome Framework -chrome_main.cc:125 ) ChromeMain 0x0000000101bc2dd3 (Google Chrome -chrome_exe_main_mac.cc:165 ) main 0x00007fff9b6df5ac (libdyld.dylib + 0x000035ac ) 0x00007fff9b6df5ac (libdyld.dylib + 0x000035ac ) Adding release blocker for this issue.Please undo if not the case. Thank You!
,
Nov 9 2017
Other steps to reproduce this issue: 1. Launch chrome, navigate to NTP and click on Avatar icon. 2. Now click on 'Sign in to chrome', enter email id and press Enter key. 3. Observe Actual: Browser gets crashed.
,
Nov 9 2017
Updating blocker priority for this issue as per comment #3.Please decrease the priority if not the case. Thank You!
,
Nov 9 2017
,
Nov 9 2017
,
Nov 9 2017
Issue 783198 has been merged into this issue.
,
Nov 9 2017
,
Nov 9 2017
I think what happens here is: 1. Theres 2 renderpasses, B=child, A=root. 2. ProcessForOverlays() turns a RPDQ into an overlay (CALayer). 3. B is skipped for drawing, for whatever reason (cached?). 4. A is skipped because skip_drawing_root_render_pass is true. 5. So UseRenderPass is never called, and current_render_pass is null. 6. ScheduleCALayers calls ScheduleRenderPassDrawQuad for the scheduled RPDQ overlay. 7. ScheduleRenderPassDrawQuad makes a resource pool, then calls CopyRenderPassDrawQuadToOverlayResource 8. CopyRenderPassDrawQuadToOverlayResource wants to know the color space of the current_render_pass. current_render_pass is still null in 8, so we crash. However, before the patch, current_render_pass would have been the child which is the incorrect renderpass, so the color space would be wrong. If ProcessForOverlays() is indeed only making overlays from the root renderpass, then this should be asking the root renderpass. Or we need to store the renderpass with the rpdq for the CALayer code.
,
Nov 9 2017
Welp. I put a NOTREACHED() in GLRenderer::CopyRenderPassDrawQuadToOverlayResource() and not a single unit test fails. :(
,
Nov 9 2017
Issue 783275 has been merged into this issue.
,
Nov 9 2017
I have seen the last error msg in Issue 783275: cc::ResourceProvider::LockForRead without having first written to the resource will result in this error.
,
Nov 9 2017
Ok I have a unit test that reproduces the CALayer mac crash. I think the LockForRead one is a bit different, and need to investigate that next.
,
Nov 9 2017
GLRenderer::SetUseProgram() also uses current_frame()->current_render_pass->color_space, which was the wrong render pass before, and null with my patch.
,
Nov 9 2017
#0 gfx::ColorSpace::IsValid (this=0xa0) at ../../ui/gfx/color_space.cc:71
#1 0x00007ffff39a51ee in viz::GLRenderer::SetUseProgram (this=0x7fffffffc478, program_key_no_color=..., src_color_space=..., dst_color_space=...)
at ../../components/viz/service/display/gl_renderer.cc:2986
#2 0x00007ffff399b6db in viz::GLRenderer::SetUseProgram (this=0x7fffffffc478, program_key=..., src_color_space=...) at ../../components/viz/service/display/gl_renderer.cc:2979
#3 0x00007ffff39a02d1 in viz::GLRenderer::ChooseRPDQProgram (this=0x7fffffffc478, params=0x7fffffff9748) at ../../components/viz/service/display/gl_renderer.cc:1408
#4 0x00007ffff39ad895 in viz::GLRenderer::CopyRenderPassDrawQuadToOverlayResource (this=0x7fffffffc478, ca_layer_overlay=0x25c1304e9720, resource=0x7fffffff9f68,
new_bounds=0x7fffffff9f58) at ../../components/viz/service/display/gl_renderer.cc:3390
,
Nov 9 2017
Issue 783443 has been merged into this issue.
,
Nov 9 2017
So a horrible fix would be to say "well, if we don't have a current render pass, then you must have meant the root render pass for the current frame"
,
Nov 9 2017
I wonder if the ReadLock one is the DCHECK in ComputeScissorRectForRenderPass() being violated.
It says:
DCHECK(
current_render_pass->copy_requests.empty() ||
(current_render_pass->damage_rect == current_render_pass->output_rect));
That is, the pass damage rect is the output rect if there's a copy. Presumably the output rect is not empty.. then the damage rect is not empty and we won't skip the pass due to ComputeScissorRectForRenderPass().IsEmpty().
If we did skip it then we won't have a texture anymore, as wutao pointed out on the CL.
GLRenderer::CopyRenderPassDrawQuadToOverlayResource() does:
cc::ScopedResource* contents_texture =
render_pass_textures_[ca_layer_overlay->rpdq->render_pass_id].get();
...
params.contents_texture = contents_texture;
So it's grabbing the draw quad's texture. If we skipped and didn't draw it, then it would crash when UpdateRPDQTexturesForSampling() does:
params->contents_resource_lock =
base::MakeUnique<cc::DisplayResourceProvider::ScopedSamplerGL>(
resource_provider_, params->contents_texture->id(), GL_LINEAR);
The id there will be 0, which will crash when ResourceProvider tries to use a thing not in the resource map.
The stack is coming from a copy request which is what made me think this
,
Nov 9 2017
> So a horrible fix would be to say "well, if we don't have a current render pass, then you must have meant the root render pass for the current frame" I did something better for the color space stuff in https://chromium-review.googlesource.com/c/chromium/src/+/760558 PS4.
,
Nov 9 2017
Issue 783271 has been merged into this issue.
,
Nov 9 2017
Oh so crash/ has some of the source indexed but not all on the other crash, but what it is is: 1. DirectRenderer::DrawFrame() calls DrawRenderPassAndExecuteCopyRequests(root_render_pass); 2. DirectRenderer::DrawRenderPassAndExecuteCopyRequests() calls DrawRenderPass() 3. ... we end up in DoDrawQuad->DrawRenderPassDrawQuad .. etc So this isn't a copy request my above theory is wrong. It's that the root RenderPass has a RPDQ to a RenderPass that got skipped. Maybe cuz it had no damage. The damage needs to consider the bigger picture in order to early out like that.
,
Nov 9 2017
The key thot about damage I have is that damage is aggregated up from children to the root. So just cuz the child has no damage doesn't mean that the root has no damage that overlaps it (or if we're not using partial swap) that promises the child won't be drawn. I need to revisit the early out when it was added to see its intentions again.
,
Nov 9 2017
I was thinking due to partial swap, the ScissorRect could be empty (the damage_rect is empty because it is out of view port for the first frame [1]), but it becomes visible later for the second frame? [1] copy_pass->damage_rect.Intersect(damage_rect_in_render_pass_space) in the code. https://cs.chromium.org/chromium/src/components/viz/service/display/surface_aggregator.cc?l=737&rcl=9fd12adae19d303c4106393d4ccdd1598355c9c5
,
Nov 10 2017
Oh I think I get the intent of the ComputeScissorRectForRenderPass(). It doesn't even check for partial swap, it wasn't meant to skip passes we don't have damage for at all. It was really only to prevent caching in some cases. Basically caching isn't used if the renderpass says it has damage in it, which is the has_damage_from_contributing_content flag. But SurfaceAggregator may have added some damage to the surface in some cases (which is https://codereview.chromium.org/1927413002 - where its saying if there's a filter then we dont know the damage, so damage everything). However problems that I see here: 1. It damages everything even if there was no damage before, if it sees any RPDQ that has a filter. 2. It damages everything if the RP has a filter, even if the RP (with filter) don't intersect the root RP. These 2 decrease out efficiency in the presence of pixel-moving filters proportional to their size when anything else on the page changes. 3. It copies damage from the root into the child, which will prevent caching from being used when anything is changing below/above the RenderPass for no reason. This one decreases our ability to make use of RenderPass cacheing. But importantly for this bug, we shouldn't skip renderpasses based on the scissor rect, only prevent cacheing.
,
Nov 10 2017
,
Nov 10 2017
And regarding caching also: We have a flag RenderPass::has_damage_from_contributing_content, which is supposed to be true if the texture for the RenderPass needs to be redrawn (ie damage is present). It ignores damage on the mask since that is applied after the RenderPass is drawn (https://cs.chromium.org/chromium/src/cc/trees/damage_tracker.cc?rcl=20b971fb48c5496b6925b84952f42c47b606e5eb&l=178) But the damage from the mask will cause damage in the root that overlaps the RenderPass (cuz thats the mask's job) so it makes this bool no different than just checking the render pass damage_rect. In otherwords the damage rect being non-empty should be a superset of the cases where has_damage_from_contributing_content is true, so as long as we dont cache when the child has any damage that intersects it in the root, there's no reason for has_damage_from_contributing_content to exist AFAICT.
,
Nov 10 2017
Actually SurfaceAggregator always makes the damage rect on the RenderPass == output_rect when |cache_render_pass| is true. So, I don't actually see how we would ever use the cached texture in that case, because of the ComputeScissorRectForRenderPass() check.
,
Nov 10 2017
,
Nov 10 2017
#27, there are only a few places turning on caching (login UI/overview mode on CrOS, window close animation, window maximization animation).
,
Nov 10 2017
Yeah, I think that this ComputeScissorRect check will make us redraw the cached surface every frame no matter what we do. At least I can't see how not. I also think it's not needed.
,
Nov 10 2017
I have a unit test to reproduce the 2nd crash: [30761:30761:1109/201807.001664:3236984305987:FATAL:display_resource_provider.cc(387)] Check failed: resource->allocated. #0 0x7f98c32edda7 base::debug::StackTrace::StackTrace() #1 0x7f98c3316111 logging::LogMessage::~LogMessage() #2 0x7f98c21d7a9e cc::DisplayResourceProvider::LockForRead() #3 0x7f98c21d82ba cc::DisplayResourceProvider::ScopedSamplerGL::ScopedSamplerGL() #4 0x7f98c188a7dd viz::GLRenderer::UpdateRPDQTexturesForSampling() #5 0x7f98c1888c1f viz::GLRenderer::DrawRenderPassQuadInternal() #6 0x7f98c1884636 viz::GLRenderer::DrawRenderPassQuad() #7 0x7f98c18835de viz::GLRenderer::DoDrawQuad() #8 0x7f98c18741f8 viz::DirectRenderer::DrawRenderPass()
,
Nov 10 2017
Oops, here's a better one :) [33660:33660:1109/203106.200077:3237763504398:FATAL:gl_renderer.cc(1113)] Check failed: contents_texture->id(). #0 0x7efd6941bda7 base::debug::StackTrace::StackTrace() #1 0x7efd69444111 logging::LogMessage::~LogMessage() #2 0x7efd679b26a8 viz::GLRenderer::DrawRenderPassQuad() #3 0x7efd679b166e viz::GLRenderer::DoDrawQuad() #4 0x7efd679a2288 viz::DirectRenderer::DrawRenderPass() #5 0x7efd679a13cb viz::DirectRenderer::DrawRenderPassAndExecuteCopyRequests() #6 0x7efd679a0edb viz::DirectRenderer::DrawFrame() The id is 0 which means it uses a resource not in the map as I was thinking. The test simply draws a frame with 2 renderpass, and no damage overlapping the child, without partial swap on.
,
Nov 10 2017
Issue 783537 has been merged into this issue.
,
Nov 10 2017
Issue 783543 has been merged into this issue.
,
Nov 10 2017
Issue 783542 has been merged into this issue.
,
Nov 10 2017
783542 has a slightly different call stack that also looks like a 0 resource id. 0x00007ffc0f0d78c6 (chrome.dll - display_resource_provider.cc: 429) cc::DisplayResourceProvider::UnlockForRead(unsigned int) 0x00007ffc0f0d7d32 (chrome.dll - display_resource_provider.cc: 473) cc::DisplayResourceProvider::ScopedSamplerGL::~ScopedSamplerGL() 0x00007ffc0e6a758d (chrome.dll - gl_renderer.cc: 187) viz::DrawRenderPassDrawQuadParams::~DrawRenderPassDrawQuadParams() 0x00007ffc0e6a4488 (chrome.dll - gl_renderer.cc: 1120) viz::GLRenderer::DrawRenderPassQuad(viz::RenderPassDrawQuad const *,gfx::QuadF const *) 0x00007ffc0e6b635e (chrome.dll - direct_renderer.cc: 595) viz::DirectRenderer::DrawRenderPass(viz::RenderPass const *) 0x00007ffc0e6b586a (chrome.dll - direct_renderer.cc: 487) viz::DirectRenderer::DrawRenderPassAndExecuteCopyRequests(viz::RenderPass *) 0x00007ffc0e6b550f (chrome.dll - direct_renderer.cc: 333) viz::DirectRenderer::DrawFrame(std::vector<std::unique_ptr<viz::RenderPass,std::default_delete<viz::RenderPass> >,std::allocator<std::unique_ptr<viz::RenderPass,std::default_delete<viz::RenderPass> > > > *,float,gfx::Size const &) 0x00007ffc0e68f0b7 (chrome.dll - display.cc: 349) viz::Display::DrawAndSwap() But in this case, somehow it managed to LockForRead, but then crashes on UnlockForRead later. I feel confident the other fixes will resolve this too, but am not sure how this would happen either.
,
Nov 10 2017
,
Nov 10 2017
void DisplayResourceProvider::UnlockForRead(viz::ResourceId id) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
ResourceMap::iterator it = resources_.find(id);
CHECK(it != resources_.end()); <--- Where that last crash happens. So its working with a resource not in the map. Somehow on lock it manages to not crash, while using the map's end() iterator, so I guess it just did some undefined stuff instead with a random Resource* pointer. There's no CHECK on the lock but there is on the unlock.
,
Nov 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/986d87a5fada7910886f1fc07e592c7698da128f commit 986d87a5fada7910886f1fc07e592c7698da128f Author: danakj <danakj@chromium.org> Date: Fri Nov 10 19:30:54 2017 When calling UseRenderPass() to restore, ensure it runs. UseRenderPass() has two use cases, either to set up a render pass as the framebuffer for drawing at the start of the draw, or to restore us back to that state after making some intermediate change to which framebuffer is bound. UseRenderPass() has some optimizations where it can return false and not do anything, if it detects that we don't want to draw the render pass. The first use case, in DrawRenderPass() makes use of this return value to avoid drawing anything. Other callers to UseRenderPass() do not check the return value because we're already drawing the pass and they are trying to restore state. If it early outs for these it would be unexpected and produce bugs. Currently DecideRenderPassAllocationsForFrame modifies render passes to try avoid such bugs, but there are other ways to encounter the bug. This changes UseRenderPass() to never early out so the latter cases will not avoid resetting the framebuffer. The former case can avoid UseRenderPass() entirely by checking CanSkipRenderPass(). If it is true, then it does not need to be drawn. Also, we will now ensure that UseRenderPass() is called after using a ScopedUseGrContext when drawing a RenderPassDrawQuad. Currently on errors it would early out before restoring the framebuffer, which could cause bugs for the next quads in the RenderPass. This was introduced when the UseRenderPass() call was moved out of the ~ScopedUseGrContext() destructor in https://codereview.chromium.org/2203033005. When skipping a RenderPass, current_render_pass is no longer set, which causes a crash in code that assumes it is, incorrectly. So this updates some paths in RenderPassDrawQuad drawing to avoid crashing, by passing the colorspace directly instead of trying to pull it off current_render_pass, since the caller knows where to find the correct one, and by using the root_render_pass instead for copying RenderPassDrawQuads to textures, since they draw into the root. This all came up via https://chromium-review.googlesource.com/c/chromium/src/+/757027#message-b6e905925e834419ff448e2093a392053d5d321a R=enne@chromium.org, wutao@chromium.org Bug: 782044 , 738190 , 782042, 783087 Change-Id: I62ea68be6b3008ec8b8370466db0e26321f022f9 Reviewed-on: https://chromium-review.googlesource.com/760558 Reviewed-by: enne <enne@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Reviewed-by: Tao Wu <wutao@chromium.org> Commit-Queue: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/master@{#515633} [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/direct_renderer.cc [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/direct_renderer.h [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer.cc [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer.h [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer_unittest.cc [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/renderer_pixeltest.cc [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/skia_renderer.cc [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/skia_renderer.h [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/software_renderer.cc [modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/software_renderer.h
,
Nov 10 2017
Issue 783551 has been merged into this issue.
,
Nov 10 2017
One more followup coming to fix caching. I added a TODO in CanSkipRenderPass(): // TODO(crbug.com/783275): It's possible to skip a child RenderPass if damage // does not overlap it, since that means nothing has changed: // ComputeScissorRectForRenderPass(render_pass).IsEmpty() // However that caused crashes where the RenderPass' texture was not present // (never seen the RenderPass before, or the texture was deleted when not used // for a frame). It could avoid skipping if there is no texture present, which // is what was done for a while, but this seems to papering over a missing // damage problem, or we're failing to understand the system wholey. // If attempted again this should probably CHECK() that the texture exists, // and attempt to figure out where the new RenderPass texture without damage // is coming from. wutao@ suggested maybe that SurfaceAggregator changes renderpass ids, which would make the texture get lost without damage. I'm not sure tho, it would be surprising cuz the RPDQ would need to change too.
,
Nov 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd829cadf22696834edf4aefc9033f5c8e272183 commit bd829cadf22696834edf4aefc9033f5c8e272183 Author: danakj <danakj@chromium.org> Date: Sat Nov 11 00:19:32 2017 Remove caching avoidance when there is a non-empty scissor. See https://chromium-review.googlesource.com/c/chromium/src/+/760558#message-83efb689457770dfabb6d7371847f06f8091dfd7 and https://bugs.chromium.org/p/chromium/issues/detail?id=783087#c24 for explanations, but tl;dr this check would break caching, and shouldn't be there. Before it was used to skip render passes regardless of caching, but incorrectly, and the unittest in https://chromium-review.googlesource.com/c/chromium/src/+/760558 demonstrates that, and fails with that check. This removes the check from the caching block. R=enne@chromium.org, wutao@chromium.org Bug: 782044 , 738190 , 782042, 783087 Change-Id: Idc5baea6da5e6d663da9c1aa23ce81a55e61168d Reviewed-on: https://chromium-review.googlesource.com/763710 Commit-Queue: danakj <danakj@chromium.org> Reviewed-by: Tao Wu <wutao@chromium.org> Cr-Commit-Position: refs/heads/master@{#515757} [modify] https://crrev.com/bd829cadf22696834edf4aefc9033f5c8e272183/components/viz/service/display/direct_renderer.cc [modify] https://crrev.com/bd829cadf22696834edf4aefc9033f5c8e272183/components/viz/service/display/gl_renderer_unittest.cc |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by db...@etouch.net
, Nov 9 2017Owner: danakj@chromium.org
Status: Assigned (was: Unconfirmed)