1325911 - Support dynamic GL Context type selection with webrender.

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Sotaro Ikeda [:sotaro]]

Current gleam and webrender does not support dynamic GL Context type selection.

Comment 1

[Sotaro Ikeda [:sotaro]]

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.

Comment 2

[Sotaro Ikeda [:sotaro]]

(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.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.)

Comment 5

[Dzmitry Malyshau [:kvark]]

Attachment: Angle patches WIP (ZIP archive)

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.

Comment 6

[Sotaro Ikeda [:sotaro]]

:kvark your approach seems enough for this bug. But the patch showed garbled characters, can you update the patch again?

Comment 7

[Dzmitry Malyshau [: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).

Comment 8

[Dzmitry Malyshau [:kvark]]

Relevant (newly created) Github issues:
https://github.com/servo/gleam/issues/103
https://github.com/servo/webrender/issues/747

Comment 9

[Sotaro Ikeda [:sotaro]]

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.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 11

[Sotaro Ikeda [:sotaro]]

I prefer runtime gl type selection. Other guys might prefer static gl type selection for simplicity.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 15

[Dzmitry Malyshau [:kvark]]

Sotaro,
I agree that run-time selection is better for the end game. Please provide the corresponding PRs into WR/gleam.

Comment 16

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 17

[Sotaro Ikeda [:sotaro]]

(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.

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Split gl::gl_type() related change

Comment 19

[Sotaro Ikeda [:sotaro]]

attachment 8828744 [details] [diff] [review], by using gl::gl_type(), target cgfs were reduced.

Comment 20

[Sotaro Ikeda [:sotaro]]

Created issue of adding gl type detection function.
  https://github.com/servo/gleam/issues/104

Comment 21

[Sotaro Ikeda [:sotaro]]

Created a pull request just for discussion.
  https://github.com/servo/gleam/pull/106

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Choose ANGLE GLES or normal GL based on pref

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Choose ANGLE GLES or normal GL based on pref

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - offscreen_gl_context

Comment 25

[Sotaro Ikeda [:sotaro]]

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

Comment 26

[Sotaro Ikeda [:sotaro]]

:emilio, can you give me an advice about how to address the build failure in Comment 25? Thanks!

Comment 27

[Sotaro Ikeda [:sotaro]]

(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.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

https://github.com/emilio/rust-offscreen-rendering-context/pull/81

Comment 30

[Sotaro Ikeda [:sotaro]]

: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.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 32

[Sotaro Ikeda [:sotaro]]

(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 :-(

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - webrender

Comment 34

[Sotaro Ikeda [:sotaro]]

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`.

Comment 35

[Sotaro Ikeda [:sotaro]]

By Comment 34, I tend to prefer using Rc.

Comment 36

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 37

[Emilio Cobos Álvarez (:emilio)]

(Not to use _in_ offscreen_gl_context itself, but to use by the consumers. Not totally sure yet.

Comment 38

[Emilio Cobos Álvarez (:emilio)]

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 :)

Comment 39

[Sotaro Ikeda [:sotaro]]

(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.

Comment 40

[Sotaro Ikeda [:sotaro]]

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.

Comment 41

[Sotaro Ikeda [:sotaro]]

: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.

Comment 42

[Sotaro Ikeda [:sotaro]]

(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)

Comment 43

[Dzmitry Malyshau [:kvark]]

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!

Comment 44

[Sotaro Ikeda [:sotaro]]

:kvark, I updated a patch of https://github.com/servo/gleam/pull/106. Can you review it again?

Comment 45

[Sotaro Ikeda [:sotaro]]

https://github.com/emilio/rust-offscreen-rendering-context/pull/81

Comment 46

[Sotaro Ikeda [:sotaro]]

https://github.com/emilio/rust-offscreen-rendering-context/issues/84

Comment 47

[Sotaro Ikeda [:sotaro]]

Attachment: patch_tmp_webrender

Comment 48

[Sotaro Ikeda [:sotaro]]

Attachment: patch_tmp_webrender

Comment 49

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - webrender

Comment 50

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - webrender

Comment 51

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - webrender

Comment 52

[Sotaro Ikeda [:sotaro]]

Attachment: rolled up patch

Comment 53

[Sotaro Ikeda [:sotaro]]

Attachment: patch for webrender

Comment 54

[Sotaro Ikeda [:sotaro]]

Attachment: patch for gecko

The patch expects https://github.com/servo/webrender/pull/965 and ./mach vendor rust.

Comment 55

[Sotaro Ikeda [:sotaro]]

Attachment: patch for gecko

Update nits.

Comment 56

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 57

[Sotaro Ikeda [:sotaro]]

Attachment: patch for gecko

Rebased.

Comment 58

[Sotaro Ikeda [:sotaro]]

Attachment: rolled up patch

Comment 59

[Sotaro Ikeda [:sotaro]]

Attachment: patch for gecko

Rebased.

Comment 60

[Sotaro Ikeda [:sotaro]]

Attachment: patch for gecko

Comment 61

[Sotaro Ikeda [:sotaro]]

Attachment: patch for gecko

Comment 62

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 63

[Sotaro Ikeda [:sotaro]]

It is OK to land the gecko patch:)

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/5974b444127a
Update Gleam to v0.4. r=kats

Comment 65

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/mozilla-central/rev/5974b444127a