Don't issue zero-instance draw calls

[kvark]

We sometimes happen to start a batch but don't end up putting any primitives to it (e.g. with B_Blend). This is bad for breaking the batch, introducing state changes, and finally triggering anger looks from the driver (https://github.com/szeged/webrender/pull/169).

[boustrophedon]

I would like to work on this. Could you give me a few pointers on where to start? It does kind of look like it was already fixed though.

[kvark]

@boustrophedon the link is to Szeged fork, and it's more of a patch than a proper fix. We need to fix it upstream, and fix for good. You can start by adding an assert to our instanced draw calls checking for instance count and seeing if it triggers on our own reftests (cd wrench && cargo run -- reftest).

[boustrophedon]

I can't build servo currently due to an older version of osmesa-src not building with newer llvms (it's being worked on). I will still look at the code though.

[emilio]

@boustrophedon you can uncomment the osmesa-src = line in components/servo/Cargo.toml

[kvark]

@boustrophedon did our reftests expose any zero-sized draw calls?

[boustrophedon]

@emilio I do not have a line that has osmesa-src in components/servo/Cargo.toml (and neither does master) so I don't know what to put on the right side of it.

@kvark I still can't build so I will try downgrading my llvm today which should allow me to build.

[kvark]

@boustrophedon oh, sorry, I thought you fail to build Servo only (as opposed to WR).

[gw3583]

@boustrophedon fwiw, you could work on this issue without servo / osmesa if you like - if you run wrench (the standalone webrender testing shell) then it might be easier to work on.

[boustrophedon]

@gw3583 Oh! I didn't know that. I will try it.

[boustrophedon]

I'm actually getting multiple reftest failures on my machine. Are they all expected to pass?

Also, it seems like running them headlessly is slower than running them via cargo run. If the headless one is running them via software rasterization through osmesa and the nonheadless one is using my graphics card that makes sense.

[gw3583]

Yup, you'll want to run it in the headless mode. This does use software rasterization, to guarantee consistency between CI machines. Even with that, it's possible you may get some local failures (for instance, if you're on a Linux distro with FreeType < 2.7.0, the default glyph rasterization is slightly different).

[boustrophedon]

I'm on freetype 2.9.1 but they are all text reftests so that makes sense. I will (finally!) start working on the code tomorrow.

Thank you everyone for the help getting me through the initial setup process!

[boustrophedon]

sorry for the delay on this (I just graduated from university 🎉). Here is a list of the reftests that create empty batches.

I'm still getting used to the codebase. Would it make sense to wrap up the args to draw_instanced_batch and draw_instanced_batch_with_previously_bound_textures in a struct, create a builder for that struct (with a bool for the textures), and return a result from the builder with an err on empty data? Then it would be handled at the callsites to draw_instanced_batched.

Or should passing empty data be handled somewhere higher up in the stack? I haven't fully digested the "life of a task" and "path to the screen" docs yet.

[kvark]

Or should passing empty data be handled somewhere higher up in the stack?

Yes. When the batching is done, issuing an empty batch would cause the previous batch to be broken, so it's already a problem. The zero instances are a symptom, not the actual problem.

[gw3583]

Apologies @boustrophedon for fixing this (https://github.com/servo/webrender/pull/2803) when you said you'd work on it - I was profiling some unrelated things and fixed this up, forgetting that there was already an issue for it.

[boustrophedon]

No problem, I had actually told @kvark that I wouldn't be available to work on it for another week.

[kvark]

@gw3583 @nical I'm sorry, but I don't see this being a proper fix for the issue. Cleaning up the batch list after the fact is a patch, it still leaves those batch cuts introduced by the primitives that never made it to the screen. I believe we should be addressing this at an earlier time, and only assert later down the road.

[nical]

I think that it doesn't leave the batch cuts because it checks whether the current batch is empty after each insertion so the empty batch is removed before it gets a chance to introduce a cut at the next primitive. That said I'm happy with fixing it at an earlier time if you have a something specific in mind.

[kvark]

Hmm indeed, upon double-checking the use of remove_unused_batches I can see how it will clean up the mess after the batch cut. This is certainly hacky and a little bit backwards, but it does the job. Thanks for correction!