Preserve depth buffer between 3D layers + optimize render order #9931

lbud · 2017-09-07T22:00:31Z

Optimizes render order by rendering all fill-extrusion textures first, so that we don't have to restore any framebuffer contents (fixes Optimize Extrusion drawing #9286)
Preserves depth buffer contents between different fill-extrusion layers in the same render pass so that multiple fill-extrusion layers can be rendered on the same map (ref Correct depth rendering for multiple fill extrusions mapbox-gl-js#4134)

Also fixes #9978 (I think)…

…ring)

lbud · 2017-09-07T23:57:11Z

😳 looks like all the various ways the different CI runs were failing were all because of d965338, so this is ready for review. A few things I want to point out:

One thing I want to flag is that I removed a few of the methods I added in the original fill-extrusion PR, as they're no longer needed here:

Context::createFramebuffer(const Texture&, const Renderbuffer<RenderbufferType::DepthComponent>&) (here) (the DepthComponent RBO type was added for extrusions)
Removed the optional OffscreenTextureAttachment type parameter from the OffscreenTexture constructor (here) / (here)

While these could be breaking changes if anything externally was using these APIs, I don't imagine they were being used anywhere other than fill-extrusion layers — do you think it's fine to remove these or do they need to be preserved just in case?

kkaefer · 2017-09-08T13:44:43Z

src/mbgl/renderer/renderer_impl.cpp

+                entry.second->startRender(parameters);
+            }
+        }
+    }


Drawing the stencil mask before the 3D pass doesn't seem necessary since we're not using it, right? Drawing the clipping mask into the stencil buffer before means that we're drawing to the FBO before the clear step, which means that the OpenGL implementation has to restore the previous frame's stencil buffer (and it might potentially produce inaccurate clipping masks).

Right — I didn't dig into startRender fully, but it looks like it both generates clipping IDs (I don't know how this works) and uploads buffers. (Without moving this step before drawing, the fill-extrusion primitive drawing runs into EXC_BAD_ACCESS here: )

And then like I said I know nothing about how the stencil drawing works but it at least reads like the actual rendering of stencil masks is still done in a separate step:

mapbox-gl-native/src/mbgl/renderer/renderer_impl.cpp

Lines 426 to 434 in 7531448

// - CLIPPING MASKS ----------------------------------------------------------------------------

// Draws the clipping masks to the stencil buffer.

{

MBGL_DEBUG_GROUP(parameters.context, "clipping masks");

static const style::FillPaintProperties::PossiblyEvaluated properties {};

static const FillProgram::PaintPropertyBinders paintAttibuteData(properties, 0);

for (const auto& clipID : parameters.clipIDGenerator.getClipIDs()) {

If that's the case, maybe copying the descriptions of this (comments + debug group) exactly is misleading and should be updated to sound more like "upload buffers + generate clipping IDs." Or does uploading and generating clipping IDs need to be separated?

Yeah, it looks like the code in the (old) CLIPPING MASKS section does a few things: startRender collects the clipping IDs (CPU code only, no OpenGL involved here), and uploads buffers (OpenGL is involved here), then proceeds to actually render the clipping mask. I think we can split this up so we have the following code flow:

Bind

Call upload/startRender

3D pass

Clear

Looking at the current code, this is already how it's set up. I assumed that startRender would actually be drawing, but that turned out to be false.

I consolidated this group with the "upload" group above it in 9e786c1.

kkaefer · 2017-09-08T13:51:10Z

src/mbgl/renderer/layers/render_fill_extrusion_layer.cpp

-        if (!imagePosA || !imagePosB) {
-            return;
+        renderTexture->bind();
+        renderTexture->attachRenderbuffer(*parameters.staticData.depthRenderbuffer);


Instead of first creating the texture and FBO (inside OffscreenTexture), validating + binding, then attaching the framebuffer (which requires revalidation), we should pass the depthRenderbuffer as an (optional) argument to OffscreenTexture construction so that we can internally call context.createFramebuffer(const Texture&, const Renderbuffer<RenderbufferType::DepthStencil>&) instead of exposing an additional bindRenderbuffer.

Additionally, it looks like the depth renderbuffer is attached on every frame (which requires revalidation on every frame). Moving it to the constructor would solve that problem as well.

kkaefer · 2017-09-08T13:53:57Z

src/mbgl/util/offscreen_texture.hpp

 class OffscreenTexture {
 public:
    OffscreenTexture(gl::Context&,
-                     Size size = { 256, 256 },
-                     OffscreenTextureAttachment type = OffscreenTextureAttachment::None);


As per above, instead of passing in a type here, we could optionally pass in a gl::Renderbuffer<gl::RenderbufferType::DepthComponent> reference (or rather have two constructors, one with and one without).

kkaefer · 2017-09-08T13:56:05Z

src/mbgl/renderer/renderer_impl.cpp

+        if (!parameters.staticData.depthRenderbuffer ||
+            parameters.staticData.depthRenderbuffer->size != size) {
+            parameters.staticData.depthRenderbuffer =
+                parameters.context.createRenderbuffer<gl::RenderbufferType::DepthComponent>(size);


Before creating any objects here, we should first check if there are even extrusion layers in the stylesheet. Otherwise, all of those resources + backend binding won't be necessary.

kkaefer · 2017-09-08T13:57:06Z

src/mbgl/gl/renderbuffer.hpp

    using type = std::integral_constant<RenderbufferType, renderbufferType>;
    Size size;
-    gl::UniqueRenderbuffer renderbuffer;
+    UniqueRenderbuffer renderbuffer;
+    bool dirty;


Can you please document what "dirty" means in this context? (or alternatively write convenience/wrapper functions around clearing the renderbuffer).

kkaefer · 2017-09-08T13:59:11Z

While these could be breaking changes if anything externally was using these APIs, I don't imagine they were being used anywhere other than fill-extrusion layers — do you think it's fine to remove these or do they need to be preserved just in case?

gl::Context is internal only, so we don't need to worry about breaking APIs.

…een loop

* Add convenience methods to clarify use of renderbuffer::dirty * Add OffscreenTexture constructor with rbo ref param so that initial framebuffer creation/binding can bind depth rbo at the same time

kkaefer · 2017-09-11T09:33:28Z

src/mbgl/renderer/renderer_impl.cpp

+    // depth rbo between them.
+    if (parameters.staticData.has3D) {
+        MBGL_DEBUG_GROUP(parameters.context, "3d");
+        parameters.backend.bind();


I believe this call isn't necessary. Binding the backend means that the default framebuffer is going to be bound and that its viewport will be set accordingly. However, in the first call to RenderFillExtrusionLayer::render(), we're calling renderTexture->bind();, which binds the OffscreenTexture's framebuffer.

kkaefer · 2017-09-11T09:34:45Z

src/mbgl/renderer/renderer_impl.cpp

+        parameters.backend.bind();
+        parameters.pass = RenderPass::Pass3D;
+
+        const auto size = parameters.context.viewport.getCurrentValue().size;


parameters.context contains the value of the current OpenGL state (unless it is dirty). Instead of using this value, we should use the canonical value from parameters.state.getSize(), since the OpenGL state's value could be dirty or set to some other value if we don't bind the backend first (see above).

@kkaefer interesting — when I change this to parameters.state.getSize() here (renderbuffer creation) and in framebuffer/texture creation

mapbox-gl-native/src/mbgl/renderer/renderer_impl.cpp

Lines 392 to 404 in 0efb0b7

if (parameters.staticData.has3D) {

MBGL_DEBUG_GROUP(parameters.context, "3d");

parameters.backend.bind();

parameters.pass = RenderPass::Pass3D;

const auto size = parameters.context.viewport.getCurrentValue().size;

if (!parameters.staticData.depthRenderbuffer ||

parameters.staticData.depthRenderbuffer->size != size) {

parameters.staticData.depthRenderbuffer =

parameters.context.createRenderbuffer<gl::RenderbufferType::DepthComponent>(size);

}

parameters.staticData.depthRenderbuffer->shouldClear(true);

mapbox-gl-native/src/mbgl/renderer/layers/render_fill_extrusion_layer.cpp

Lines 54 to 59 in 0efb0b7

if (parameters.pass == RenderPass::Pass3D) {

const auto size = parameters.context.viewport.getCurrentValue().size;

if (!renderTexture || renderTexture->getSize() != size) {

renderTexture = OffscreenTexture(parameters.context, size, *parameters.staticData.depthRenderbuffer);

}

on my iPhone, it appears you're right that this value is different, but that the dimensions of parameters.state.getSize() are half those of the others (non-retina). If I change them, fill-extrusion layers degrade to non-retina quality…

kkaefer · 2017-09-11T09:37:24Z

src/mbgl/gl/context.cpp

    checkFramebuffer();
-    return { depthTarget.size, std::move(fbo) };
+    return { color.size, std::move(fbo) };


Only formatting/variable names changed here (except for size equality check); can we avoid this change please?

… backend in 3D pass

lbud · 2017-09-11T22:34:29Z

@kkaefer (commenting here rather than inline as they're getting collapsed and hard to find):

In the 3D pass (permalinking HEAD~1 here):

mapbox-gl-native/src/mbgl/renderer/renderer_impl.cpp

Lines 389 to 416 in 0efb0b7

    
           // - 3D PASS ------------------------------------------------------------------------------------- 
        
           // Renders any 3D layers bottom-to-top to unique FBOs with texture attachments, but share the same 
        
           // depth rbo between them. 
        
           if (parameters.staticData.has3D) { 
        
               MBGL_DEBUG_GROUP(parameters.context, "3d"); 
        
               parameters.backend.bind(); 
        
               parameters.pass = RenderPass::Pass3D; 
        
               const auto size = parameters.context.viewport.getCurrentValue().size; 
        
               if (!parameters.staticData.depthRenderbuffer || 
        
                   parameters.staticData.depthRenderbuffer->size != size) { 
        
                   parameters.staticData.depthRenderbuffer = 
        
                       parameters.context.createRenderbuffer<gl::RenderbufferType::DepthComponent>(size); 
        
               } 
        
               parameters.staticData.depthRenderbuffer->shouldClear(true); 
        
               uint32_t i = static_cast<uint32_t>(order.size()) - 1; 
        
               for (auto it = order.begin(); it != order.end(); ++it, --i) { 
        
                   parameters.currentLayer = i; 
        
                   if (it->layer.hasRenderPass(parameters.pass)) { 
        
                       MBGL_DEBUG_GROUP(parameters.context, it->layer.getID()); 
        
                       it->layer.render(parameters, it->source); 
        
                   } 
        
               } 
        
               // The main backend/framebuffer will be rebound in the clear step. 
        
           }

In live rendering, changing parameters.context.viewport.getCurrentValue().size to parameters.state.getSize() causes non-retina (degraded) rendering for fill-extrusion layers
In headless rendering, failing to bind the backend before calling parameters.context.viewport.getCurrentValue().size throws an error because the viewport is uninitialized, so size = {0, 0}. On the other hand using parameters.state.getSize() without binding the backend allows all tests to pass.

🤔

kkaefer · 2017-09-13T12:28:56Z

In live rendering, changing parameters.context.viewport.getCurrentValue().size to parameters.state.getSize() causes non-retina (degraded) rendering for fill-extrusion layers

Good point. We have two different sizes; the canonical size, and the actual framebuffer size. In order to render at the correct resolution, we need the framebuffer size. In your initial code, you obtained that from parameters.context.viewport.getCurrentValue().size, but as you noted, that only works when setting the backend (which also updates this value). However, this is a bad idea since we bind the screen first just to unbind and bind the extrusions FBO in the next step. What we'd need here is a way to obtain the viewport size without actually calling bind. Since the only place that knows about the correct viewport size is the Backend, we'd need to add a call to backend that allows retrieving the size without going through OpenGL state.

…t get destroyed when removing a layer

…inding or changing state

lbud · 2017-09-19T00:20:39Z

I think this is ready — @kkaefer if you agree then I'll ping a few mobile folks for 👀 on the small platform-specific changes (getFramebufferSize) here.

lbud · 2017-09-20T18:46:53Z

@tobrun or @Guardiola31337 — can I bug one of you for a quick glance at the Android files changed here? It's pretty straightforward but I have zero Android experience/devices to test on.

lbud · 2017-09-20T20:26:36Z

@brunoabinader can I bug you for a quick glance at the Qt changes here? They're pretty straightforward but I haven't tested them on any Qt device 😬

boundsj · 2017-09-20T21:44:09Z

Tagging @fabian-guerra for iOS.

@lbud I think the changes in MGLMapView are small. Do you need @fabian-guerra to help test on device?

lbud · 2017-09-20T22:02:03Z

Thanks @boundsj! I tested on my own 🍎 devices so I feel comfortable with the ios/macos changes functionally.

tobrun

Tested on 3 Android devices, didn't notice any regressions.

brunoabinader

Working great on Qt! 🚢

brunoabinader · 2017-09-21T13:54:45Z

src/mbgl/gl/context.cpp

    auto fbo = createFramebuffer();
    bindFramebuffer = fbo;
+    // TODO: revert this; just for debugging on CI:
+    GLuint depthInt = depthTarget.renderbuffer;
+    Log::Info(Event::General, "bound framebuffer: %d; renderbuffer name: %d", bindFramebuffer, depthInt);


Got this on Qt app's stdout ⚠️

oops 🙈 good catch, thanks @brunoabinader!

fabian-guerra

iOS/macOS code looks good to me. 👍🏼

Lauren Budorick added 4 commits September 7, 2017 13:55

Share single depth renderbuffer between fill-extrusion textures

fb9c316

Following rebase, remove references to renderTree

736359a

Add default constructor for renderbuffer

aa38666

Bind backend before doing any 3D rendering (needed for headless rende…

d965338

…ring)

lbud changed the title ~~[not ready] Preserve depth buffer between 3D layers + optimize render order~~ Preserve depth buffer between 3D layers + optimize render order Sep 7, 2017

lbud requested review from kkaefer and jfirebaugh September 7, 2017 23:57

Un-ignore fill-extrusion-multiple

7531448

kkaefer suggested changes Sep 8, 2017

View reviewed changes

Lauren Budorick added 2 commits September 8, 2017 13:16

Check for presence of 3D layers before entering fill-extrusion offscr…

044617d

…een loop

Per review:

0efb0b7

* Add convenience methods to clarify use of renderbuffer::dirty * Add OffscreenTexture constructor with rbo ref param so that initial framebuffer creation/binding can bind depth rbo at the same time

kkaefer reviewed Sep 11, 2017

View reviewed changes

kkaefer added Core The cross-platform C++ core, aka mbgl rendering labels Sep 11, 2017

Revert createFramebuffer to master state as relevant; don't bind main…

4af7bfa

… backend in 3D pass

Lauren Budorick added 2 commits September 11, 2017 16:19

Restore backend binding to get contextual size information

e3c999a

Add debug output for failing CI build

84ec9e2

Lauren Budorick added 2 commits September 13, 2017 11:06

Store depth RBO as ptr on OffscreenTexture so that shared RBO does no…

615328a

…t get destroyed when removing a layer

Add getFramebufferSize methods to get real framebuffer size without b…

f0fa915

…inding or changing state

lbud force-pushed the preserve-depth branch from 6f51101 to f0fa915 Compare September 14, 2017 00:06

Clarify debug groups by consolidating (upload / 'clip')

9e786c1

lbud mentioned this pull request Sep 19, 2017

Shuffling causes fill-extrusion-color/property-function to fail #9978

Closed

brunoabinader mentioned this pull request Sep 20, 2017

[build] Shuffle render tests in 'test-node-recycle-map' target #10032

Merged

kkaefer approved these changes Sep 20, 2017

View reviewed changes

boundsj requested a review from fabian-guerra September 20, 2017 21:43

tobrun approved these changes Sep 21, 2017

View reviewed changes

brunoabinader approved these changes Sep 21, 2017

View reviewed changes

brunoabinader reviewed Sep 21, 2017

View reviewed changes

fabian-guerra approved these changes Sep 21, 2017

View reviewed changes

revert debug logging

f0cabcd

lbud merged commit a9ddf5b into master Sep 21, 2017

lbud deleted the preserve-depth branch September 21, 2017 21:26

jfirebaugh mentioned this pull request Oct 11, 2017

Preserve depth buffer between fill-extrusion layers #9894

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve depth buffer between 3D layers + optimize render order #9931

Preserve depth buffer between 3D layers + optimize render order #9931

lbud commented Sep 7, 2017 •

edited

Loading

lbud commented Sep 7, 2017

kkaefer Sep 8, 2017

lbud Sep 8, 2017 •

edited

Loading

kkaefer Sep 11, 2017 •

edited

Loading

lbud Sep 19, 2017

kkaefer Sep 8, 2017

kkaefer Sep 8, 2017

kkaefer Sep 8, 2017

kkaefer Sep 8, 2017

kkaefer commented Sep 8, 2017

kkaefer Sep 11, 2017

kkaefer Sep 11, 2017

lbud Sep 11, 2017

kkaefer Sep 11, 2017

lbud commented Sep 11, 2017

kkaefer commented Sep 13, 2017

lbud commented Sep 19, 2017

lbud commented Sep 20, 2017

lbud commented Sep 20, 2017

boundsj commented Sep 20, 2017

lbud commented Sep 20, 2017

tobrun left a comment

brunoabinader left a comment

brunoabinader Sep 21, 2017

lbud Sep 21, 2017

fabian-guerra left a comment

	// - CLIPPING MASKS ----------------------------------------------------------------------------
	// Draws the clipping masks to the stencil buffer.
	{
	MBGL_DEBUG_GROUP(parameters.context, "clipping masks");

	static const style::FillPaintProperties::PossiblyEvaluated properties {};
	static const FillProgram::PaintPropertyBinders paintAttibuteData(properties, 0);

	for (const auto& clipID : parameters.clipIDGenerator.getClipIDs()) {

	if (parameters.staticData.has3D) {
	MBGL_DEBUG_GROUP(parameters.context, "3d");
	parameters.backend.bind();
	parameters.pass = RenderPass::Pass3D;

	const auto size = parameters.context.viewport.getCurrentValue().size;

	if (!parameters.staticData.depthRenderbuffer \|\|
	parameters.staticData.depthRenderbuffer->size != size) {
	parameters.staticData.depthRenderbuffer =
	parameters.context.createRenderbuffer<gl::RenderbufferType::DepthComponent>(size);
	}
	parameters.staticData.depthRenderbuffer->shouldClear(true);

	if (parameters.pass == RenderPass::Pass3D) {
	const auto size = parameters.context.viewport.getCurrentValue().size;

	if (!renderTexture \|\| renderTexture->getSize() != size) {
	renderTexture = OffscreenTexture(parameters.context, size, *parameters.staticData.depthRenderbuffer);
	}

Preserve depth buffer between 3D layers + optimize render order #9931

Preserve depth buffer between 3D layers + optimize render order #9931

Conversation

lbud commented Sep 7, 2017 • edited Loading

lbud commented Sep 7, 2017

Choose a reason for hiding this comment

lbud Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

kkaefer Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkaefer commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud commented Sep 11, 2017

kkaefer commented Sep 13, 2017

lbud commented Sep 19, 2017

lbud commented Sep 20, 2017

lbud commented Sep 20, 2017

boundsj commented Sep 20, 2017

lbud commented Sep 20, 2017

tobrun left a comment

Choose a reason for hiding this comment

brunoabinader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-guerra left a comment

Choose a reason for hiding this comment

lbud commented Sep 7, 2017 •

edited

Loading

lbud Sep 8, 2017 •

edited

Loading

kkaefer Sep 11, 2017 •

edited

Loading