Closed Bug 1325911 Opened 3 years ago Closed 3 years ago

Support dynamic GL Context type selection with webrender.

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 20 obsolete files)

291.80 KB, patch
Details | Diff | Splinter Review
8.34 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Current gleam and webrender does not support dynamic GL Context type selection.
Assignee: nobody → sotaro.ikeda.g
webrender depends on offscreen_gl_context. offscreen_gl_context also uses gleam. gecko does not use offscreen_gl_context, therefore we do not have a necessity to add "dynamic GL Context type selection" to offscreen_gl_context. Instread, it seems better if webrender has a capability to remove offscreen_gl_context.
(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> webrender depends on offscreen_gl_context. offscreen_gl_context also uses
> gleam. gecko does not use offscreen_gl_context, therefore we do not have a
> necessity to add "dynamic GL Context type selection" to
> offscreen_gl_context. Instread, it seems better if webrender has a
> capability to remove offscreen_gl_context.

Hmm, it seems simpler to keep offscreen_gl_context.
Depends on: 1331541
Depends on: 1331546
Attached patch wip patch (obsolete) — Splinter Review
It seems like it would be worth it to modify the generator to auto-generate the Rust wrappers too, with some simple rules for changing names etc.  (The functions where we actually modify the signature etc. should still be hand-written.)
Attached file Angle patches WIP (ZIP archive) (obsolete) —
We tried to hack it here with Glenn and got it running with RenderDoc capturing successfully. For some reason, the coordinate space is flipped at Y, even though Angle should take care of that.

We didn't switch from the global gl generator to the struct one. I agree that the struct may be more idiomatic and even beneficial in the long term, but I'm not sure if it's worth converting everything for Angle (in the scope of this bug), since this is certainly not required.

Our approach would just need the Android path of both Gleam and WebRender to be converted to a compile-time feature, plus a few platform-independent shader bugs that we'll do regardless.
:kvark your approach seems enough for this bug. But the patch showed garbled characters, can you update the patch again?
Flags: needinfo?(kvark)
Comment on attachment 8828116 [details]
Angle patches WIP (ZIP archive)

Oh, sorry about that! I'm still figuring out the proper way to work with BugZilla. The patch is a ZIP having separate patches for glutin-0.7.2, gleam-0.2.30, and WR).
Attachment #8828116 - Attachment description: Angle patches WIP → Angle patches WIP (ZIP archive)
Attachment #8828116 - Attachment is patch: false
Flags: needinfo?(kvark)
Thanks! attachment 8828116 [details] changes GL type by feature. It decides the GL type during build time. I thought that for quantum, it is nice if we could change gl type dynamically. Then created this bug. StructGenerator is used since it seems to fit to the situation of dynamic Gl type selection.
With the patch, gecko graphic branch could choose Choose ANGLE  GLES or normal GL based on pref.
Attachment #8827348 - Attachment is obsolete: true
I prefer runtime gl type selection. Other guys might prefer static gl type selection for simplicity.
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> Created attachment 8828250 [details] [diff] [review]
> wip patch - Choose ANGLE  GLES or normal GL based on pref
> 
> With the patch, gecko graphic branch could choose Choose ANGLE  GLES or
> normal GL based on pref.

Is disabling the GPU process necessary? I don't want to regress that because it will mean all windows (even non-webrender windows) get forced into the UI process in QR builds.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> 
> Is disabling the GPU process necessary? I don't want to regress that because
> it will mean all windows (even non-webrender windows) get forced into the UI
> process in QR builds.

It is not intrinsic to the bug. But without it caused the crash during starting firefox. I suspect that I is a problem around GLContextProviderEGL. I did not look into it yet. Similar problem for native windows GL context was addressed recently by other bug.
Ah, I see. Randall has patches in bug 1329362 for GLContextProviderEGL, but I'm not sure if they only get it working for Android, or if it works for Windows as well. Might be worth taking a look.
Sotaro,
I agree that run-time selection is better for the end game. Please provide the corresponding PRs into WR/gleam.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Ah, I see. Randall has patches in bug 1329362 for GLContextProviderEGL, but
> I'm not sure if they only get it working for Android, or if it works for
> Windows as well. Might be worth taking a look.

We needs bug 1329362 also for windows ANGLE. GLContextProviderEGL::CreateForCompositorWidget() requests RealWidget(), it returns nulptr when GPU process is enabled.

https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#713
(In reply to Dzmitry Malyshau [:kvark] from comment #15)
> Sotaro,
> I agree that run-time selection is better for the end game. Please provide
> the corresponding PRs into WR/gleam.

Thanks. I am going to create PRs.
attachment 8828744 [details] [diff] [review], by using gl::gl_type(), target cgfs were reduced.
Created issue of adding gl type detection function.
  https://github.com/servo/gleam/issues/104
Attachment #8828744 - Attachment is obsolete: true
Created a pull request just for discussion.
  https://github.com/servo/gleam/pull/106
Blocks: 1337625
Attachment #8828250 - Attachment is obsolete: true
Attachment #8835233 - Attachment is obsolete: true
Attached patch wip patch - offscreen_gl_context (obsolete) — Splinter Review
With attachment 8836227 [details] [diff] [review] and https://github.com/servo/gleam/pull/106/files, offscreen_gl_context build was failed with the following error log. Compiler's help message did not work: -(


 0:01.81 error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
 0:01.81    --> /home/sotaro/quantum_gfx_src_8/graphics/third_party/rust/offscreen_gl_context/src/gl_context.rs:200:70
 0:01.81     |
 0:01.81 200 |         self.draw_buffer = Some(try!(DrawBuffer::new(&self, self.gl_.as_ref(), size, color_attachment_type)));
 0:01.81     |                                                                      ^^^^^^
 0:01.81     |
 0:01.81 help: consider using an explicit lifetime parameter as shown: fn create_draw_buffer(&'a mut self, size: Size2D<i32>,
 0:01.81                       color_attachment_type: ColorAttachmentType)
 0:01.81  -> Result<(), &'static str>
 0:01.81    --> /home/sotaro/quantum_gfx_src_8/graphics/third_party/rust/offscreen_gl_context/src/gl_context.rs:199:5
 0:01.81     |
 0:01.81 199 |       fn create_draw_buffer(&mut self, size: Size2D<i32>, color_attachment_type: ColorAttachmentType) -> Result<(), &'static str> {
 0:01.81     |  _____^ starting here...
 0:01.81 200 | |         self.draw_buffer = Some(try!(DrawBuffer::new(&self, self.gl_.as_ref(), size, color_attachment_type)));
 0:01.81 201 | |         Ok(())
 0:01.81 202 | |     }
 0:01.81     | |_____^ ...ending here
 0:01.81 
 0:01.83 error: aborting due to previous error
:emilio, can you give me an advice about how to address the build failure in Comment 25? Thanks!
Flags: needinfo?(emilio+bugs)
(In reply to Sotaro Ikeda [:sotaro] from comment #26)
> :emilio, can you give me an advice about how to address the build failure in
> Comment 25? Thanks!

attachment 8836017 [details] [diff] [review] uses Rc to share ref to Gl. But if Rc is not used, I am not sure how to address the compile failure.
So the thing why it doesn't work is because even though our usage is fine (because GLContext::gl_ is effectively immutable), you can't have a reference to it while the owner has a Box, because theoretically, the owner (GLContext in this case) may mutate it while the buffer is reading.

The way to do it is either refcount it, or use a bit of unsafety. Given the context completely own both the struct and the buffer, I think it's justified to use a bit of unsafety here, I'll prepare a patch.
Flags: needinfo?(emilio+bugs)
:emilio, thanks so much! Which do you prefer to use in webrender? refcount it, or use a bit of unsafety?

I tend to prefer refcount way because of safety. If gleam usage is only for offscreen_gl_context, a bit of unsafety seems ok. But gleam is used also for webrender.
Flags: needinfo?(emilio+bugs)
For unsafety, I meant just the raw pointer bits I added with a bunch of comments in https://github.com/emilio/rust-offscreen-rendering-context/pull/81. Gleam shouldn't change anything.

In that case it's fine because the vtable is immutable, and the buffer never outlives the context.

I don't think that's related to WR at all though, does WR suffer from similar lifetime issues as offscreen_gl_context?


Seems like in WR the context can live in the Device fairly easily as a Box?
Flags: needinfo?(emilio+bugs) → needinfo?(sotaro.ikeda.g)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #31)
> For unsafety, I meant just the raw pointer bits I added with a bunch of
> comments in
> https://github.com/emilio/rust-offscreen-rendering-context/pull/81. Gleam
> shouldn't change anything.
> 
> In that case it's fine because the vtable is immutable, and the buffer never
> outlives the context.
> 
> I don't think that's related to WR at all though, does WR suffer from
> similar lifetime issues as offscreen_gl_context?

Yes, I think so. There a lot of patterns that object needs to hold ref to Gl like DrawBuffer.

> 
> 
> Seems like in WR the context can live in the Device fairly easily as a Box?

When I tried, I faced a lot of build errors :-(
Flags: needinfo?(sotaro.ikeda.g)
Attached patch wip patch - webrender (obsolete) — Splinter Review
With attachment 8836696 [details] [diff] [review], I faced the following lifetime related errors, it is caused by same reason as offscreen_gl_context case.

-----------------------------------------------------------------
 0:02.26 error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
 0:02.26    --> /home/sotaro/quantum_gfx_src_8/graphics/gfx/webrender/src/device.rs:688:17
 0:02.26     |
 0:02.26 688 |                 GpuMarker {
 0:02.26     |                 ^^^^^^^^^
 0:02.26 
 0:02.26 error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
 0:02.26    --> /home/sotaro/quantum_gfx_src_8/graphics/gfx/webrender/src/device.rs:693:17
 0:02.26     |
 0:02.26 693 |                 GpuMarker {
 0:02.26     |                 ^^^^^^^^^
 0:02.26 
 0:02.28 error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
 0:02.28     --> /home/sotaro/quantum_gfx_src_8/graphics/gfx/webrender/src/device.rs:1066:31
 0:02.28      |
 0:02.28 1066 |                 gl_: self.gl_.as_ref(),
 0:02.28      |                               ^^^^^^
 0:02.28 
 0:02.30 error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
 0:02.30     --> /home/sotaro/quantum_gfx_src_8/graphics/gfx/webrender/src/device.rs:1398:27
 0:02.30      |
 0:02.30 1398 |             gl_: self.gl_.as_ref(),
 0:02.30      |                           ^^^^^^
 0:02.30 
 0:02.32 error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
 0:02.32     --> /home/sotaro/quantum_gfx_src_8/graphics/gfx/webrender/src/device.rs:1737:27
 0:02.32      |
 0:02.32 1737 |             gl_: self.gl_.as_ref(),
 0:02.32      |                           ^^^^^^
 0:02.32 
 0:02.49 libmedia_libogg.a.desc
 0:03.29 error: aborting due to 5 previous errors
 0:03.29 
 0:03.30 error: Could not compile `webrender`.
By Comment 34, I tend to prefer using Rc.
Wait a second, since I think Rc is going to be quite a pain to use in offscreen_gl_context. Let me try to find a better solution.
(Not to use _in_ offscreen_gl_context itself, but to use by the consumers. Not totally sure yet.
I can't apply that patch to current webrender trunk (with the paths adjusted), but from comment 34 I believe most of the errors if not all are due to GpuMarker, right?

If so, I think you could make something like:

GpuMarker::with("foo", self, |device| {

})

work, which would be something like (simplified):

pub fn with<R, F>(name: &str, device: &mut Device, f: F) -> R
    where F: FnOnce(&mut Device) -> R,
{
    device.gl().push_marker();
    let ret = f(device);
    device.gl().pop_marker();
    ret
}

Though I don't know what would kvark or gw.

Even if your patch worked (which can be made to work taking the reference to the functions directly), I believe you'd have borrow checker errors due to having a reference to the functions in the marker (which would prevent you from mutating the rest of the device).

Given the GpuMarkers are objects on the stack, I believe it's not such a big deal for them to contain a raw pointer, but if you think that it is and want to refcount the functions that's fine, I'll manage to get the other stuff working on my side with Rc :)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #38)
> I can't apply that patch to current webrender trunk (with the paths
> adjusted),

Sorry, I am working on graphics branch.

> but from comment 34 I believe most of the errors if not all are
> due to GpuMarker, right?

The pasted log shows erros around GpuMarker, but GpuProfiler, GpuFrameProfile, GpuProfiler, GpuMarker, Texture, Program and VAO has lifetime problem because of referring Box.
The following seems to related to comment #28, since Box holds raw pointer.

  https://doc.rust-lang.org/nomicon/references.html#aliasing

------------------------------------

In addition to references, Rust has raw pointers: *const T and *mut T. Raw pointers have no inherent ownership or aliasing semantics. As a result, Rust makes absolutely no effort to track that they are used correctly, and they are wildly unsafe.
Depends on: 1339313
:kvark, can you comment to  https://github.com/servo/gleam/pull/106? And can you give me a code snip about how to use GlTyped? I am still not sure how to use GlTyped extension trait from Gl trait, since client only own Gl trait object.


> No. My proposal to have GlTyped extension trait assumed that any clients that work with the trait object would only use Gl, while the clients working with it generically will have the convenience of GlTyped.
Flags: needinfo?(kvark)
(In reply to Sotaro Ikeda [:sotaro] from comment #41)
> :kvark, can you comment to  https://github.com/servo/gleam/pull/106? And can
> you give me a code snip about how to use GlTyped? I am still not sure how to
> use GlTyped extension trait from Gl trait, since client only own Gl trait
> object.

At first, I thought to use ufcs.
  > https://doc.rust-lang.org/book/ufcs.html

But the following code caused error"the trait `gleam::gl::GlTyped` is not implemented for `gleam::gl".

> gl::GlTyped::buffer_data(ctx.get_gl().as_ref(), buffer_type, &data, usage)
Sotaro, you can't downcast `Gl` to `GlTyped`. So when I suggested `GlTyped` extension I thought that the clients would use it generically when they can, and just use `Gl` when they have to have a trait object. It very much depends on how you are using it, so you probably see better what works and what does not.

I commented on the gleam PR, please let me know if there is anything else pending!
Flags: needinfo?(kvark)
:kvark, I updated a patch of https://github.com/servo/gleam/pull/106. Can you review it again?
Flags: needinfo?(kvark)
Flags: needinfo?(kvark)
Attached patch patch_tmp_webrender (obsolete) — Splinter Review
Attachment #8836696 - Attachment is obsolete: true
Attachment #8836227 - Attachment is obsolete: true
Attached patch patch_tmp_webrender (obsolete) — Splinter Review
Attachment #8841859 - Attachment is obsolete: true
Attached patch wip patch - webrender (obsolete) — Splinter Review
Attachment #8842247 - Attachment is obsolete: true
Attachment #8842304 - Attachment description: tmp patch - webrender → wip patch - webrender
Attached patch wip patch - webrender (obsolete) — Splinter Review
Attachment #8842304 - Attachment is obsolete: true
Attached patch wip patch - webrender (obsolete) — Splinter Review
Attachment #8844296 - Attachment is obsolete: true
Attachment #8836017 - Attachment is obsolete: true
Attached patch rolled up patch (obsolete) — Splinter Review
Attachment #8828116 - Attachment is obsolete: true
Attached patch patch for webrender (obsolete) — Splinter Review
Attachment #8844306 - Attachment is obsolete: true
Attached patch patch for gecko (obsolete) — Splinter Review
The patch expects https://github.com/servo/webrender/pull/965 and ./mach vendor rust.
Attachment #8844814 - Flags: review?(bugmail)
Attached patch patch for gecko (obsolete) — Splinter Review
Update nits.
Attachment #8844814 - Attachment is obsolete: true
Attachment #8844814 - Flags: review?(bugmail)
Attachment #8844816 - Flags: review?(bugmail)
Comment on attachment 8844816 [details] [diff] [review]
patch for gecko

Review of attachment 8844816 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. When we pull the next WR update with your PR I'll make sure to include this patch.
Attachment #8844816 - Flags: review?(bugmail) → review+
Attached patch patch for gecko (obsolete) — Splinter Review
Rebased.
Attachment #8844816 - Attachment is obsolete: true
Attachment #8846461 - Flags: review+
Attachment #8844813 - Attachment is obsolete: true
Attached patch rolled up patchSplinter Review
Attachment #8844812 - Attachment is obsolete: true
Attached patch patch for gecko (obsolete) — Splinter Review
Rebased.
Attachment #8846461 - Attachment is obsolete: true
Attachment #8846990 - Flags: review+
Attached patch patch for gecko (obsolete) — Splinter Review
Attachment #8846990 - Attachment is obsolete: true
Attachment #8847448 - Flags: review+
Attached patch patch for geckoSplinter Review
Attachment #8847448 - Attachment is obsolete: true
Attachment #8847449 - Flags: review+
Blocks: 1347442
Blocks: 1348209
Is it ok if I land the gecko patch to bump the gleam version, along with a WR update? Or do you want me to wait until you have the whole angle change ready?
It is OK to land the gecko patch:)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/5974b444127a
Update Gleam to v0.4. r=kats
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.