1347641 - BlobImageRenderer integration

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Nicolas Silva [:nical]]

The goal here is to add the necessary glue to interact with WebRender's BlobImageRenderer feature (the serialization and rendering code will go in another bug).

Comment 1

[Nicolas Silva [:nical]]

Attachment: Add BytSlice to pass const slices across ffi.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Add MutBytSlice to pass mutable slices across ffi.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8847695 [details] [diff] [review]
Add BytSlice to pass const slices across ffi.

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

::: gfx/webrender_bindings/src/bindings.rs
@@ +57,5 @@
>  check_ffi_type!(_namespace_id_repr struct IdNamespace as (u32));
>  
>  #[repr(C)]
> +pub struct ByteSlice<'l> {
> +    buffer: &'l u8,

I wonder if this would be better as *mut u8. and use slice.as_ptr() in the constructor. It seems like that matches better with from_raw_parts()

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8847695 [details] [diff] [review]
Add BytSlice to pass const slices across ffi.

:mystor what do you think?

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8847695 [details] [diff] [review]
Add BytSlice to pass const slices across ffi.

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

::: gfx/webrender_bindings/WebRenderTypes.h
@@ +346,5 @@
>    bool mOwned;
>  };
>  
> +inline WrByteSlice RangeToByteSlice(mozilla::Range<uint8_t> aRange) {
> +  return WrByteSlice { &aRange[0], aRange.length() };

Use aRange.begin().get() instead of &aRange[0] here, it seems more clear to me.

::: gfx/webrender_bindings/src/bindings.rs
@@ +57,5 @@
>  check_ffi_type!(_namespace_id_repr struct IdNamespace as (u32));
>  
>  #[repr(C)]
> +pub struct ByteSlice<'l> {
> +    buffer: &'l u8,

Use *const u8 here instead of &'l u8. (I am suggesting *const as this appears to be an immutable slice).

If you want to have a lifetime reference, add a 3rd field _marker: PhantomData<&'l [u8]> to make it compile (as we need to support the reference).

I also think that this type should have a deref implementation, as it will make calling methods on the ByteSlice as though it is a slice nicer.

impl<'l> Deref for ByteSlice<'l> {
    type Target = [u8],
    fn deref(&self) -> &[u8] {
        self.as_slice()
    }
}

@@ +63,5 @@
> +}
> +
> +impl<'l> ByteSlice<'l> {
> +    pub fn new(slice: &[u8]) -> ByteSlice {
> +        ByteSlice { buffer: &slice[0], len: slice.len() }

&slice[0] is really gross...

@@ +66,5 @@
> +    pub fn new(slice: &[u8]) -> ByteSlice {
> +        ByteSlice { buffer: &slice[0], len: slice.len() }
> +    }
> +
> +    pub unsafe fn as_slice(&self) -> &[u8] { slice::from_raw_parts(self.buffer, self.len) }

I don't see why this is unsafe, the fields of this struct are private and should only be provided either from the adjacent `new` function or over FFI (which has to hold validity anyways). I think this should not be marked as unsafe for that reason. Perhaps it would be better in a submodule however, so that the code which uses it is unable to see the fields.

In addition, I think this method should tie the lifetime of the output slice to the 'l lifetime parameter, as that is the lifetime of the underlying data. for example:

pub fn as_slice(&self) -> &'l [u8] {
    unsafe { slice::from_raw_parts(self.buffer, self.len) }
}

@@ +674,3 @@
>      api.add_image(image_key,
>                    descriptor.to_descriptor(),
> +                  ImageData::new(copied_bytes),

This could be written with my suggested changes as ImageData::new(bytes.to_owned()) which seems nicer to me.

@@ +720,2 @@
>  
> +    api.update_image(key, descriptor.to_descriptor(), copied_bytes);

Same here as above.

::: gfx/webrender_bindings/webrender_ffi.h
@@ +65,5 @@
> +// FFI-safe slice of bytes. Use this accross the FFI boundary to pass a temporary
> +// view of a buffer of bytes.
> +// The canonical gecko equivalent is mozilla::Range<uint8_t>.
> +struct WrByteSlice {
> +  uint8_t* mBuffer;

This should be marked const to match with the definition in rust.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8847695 [details] [diff] [review]
Add BytSlice to pass const slices across ffi.

Also please fix the typo in the commit message :)

Comment 7

[Nicolas Silva [:nical]]

Attachment: WIP: BlobImageRenderer ffi integration

Comment 8

[Nicolas Silva [:nical]]

> Use *const u8 here instead of &'l u8. (I am suggesting *const as this appears to be an immutable slice).
> 
> If you want to have a lifetime reference, add a 3rd field _marker: PhantomData<&'l [u8]> to make it
> compile (as we need to support the reference).

Interesting, could you elaborate on the advantages of *const u8 over &u8 here? The intent is to have a slice type that is passed with the same semantics as a &[u8] (temporary and not aliased) so my hunch was that using &'l u8 was what most closely modeled the intent.

In fact it would be nice if the representation of &[u8] was stable and ffi-compatibe, I'd rather pass that directly to clearly express the intent.

> I don't see why this is unsafe, the fields of this struct are private and should only be provided either from the adjacent `new` function or over FFI (which has to hold validity anyways). I think this should not be marked as unsafe for that reason. Perhaps it would be better in a submodule however, so that the code which uses it is unable to see the fields.

as_slice is totally not safe, I much prefer the unsafe block to be where we are taking the responsibility of insuring safety, which is in each ffi function where we use as_slice.

> In addition, I think this method should tie the lifetime of the output slice to the 'l lifetime parameter, as that is the lifetime of the underlying data.

I am not sure I totally grasp the exact difference between the two. My understanding is that as it currently is, the lifetime of the output slice is a subset of 'l, isn't this the desired behavior?

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nicolas Silva [:nical] from comment #8)
> > Use *const u8 here instead of &'l u8. (I am suggesting *const as this appears to be an immutable slice).
> > 
> > If you want to have a lifetime reference, add a 3rd field _marker: PhantomData<&'l [u8]> to make it
> > compile (as we need to support the reference).
> 
> Interesting, could you elaborate on the advantages of *const u8 over &u8
> here? The intent is to have a slice type that is passed with the same
> semantics as a &[u8] (temporary and not aliased) so my hunch was that using
> &'l u8 was what most closely modeled the intent.

&'l u8 is a safe rust type which carries a lot of meaning with it beyond just "a pointer to some data which contains at least one u8 object". It states that it is a non-exclusive, freezing (no other code can modify it) reference with a lifetime of 'l which references exactly one u8 which is correctly aligned.
I don't think that it is necessarily going to trigger UB to read outside of the bounds of this &'l but it does seem to be at least bad style. Usually when we're doing something like this where the _actual_ meaning of the reference is more complex than a simple reference, it is better to use a raw pointer, because rustc doesn't make any assumptions about it.

The PhantomData<&'l [u8]> will then act to make sure that the object has the correct variance on the type parameters which is important for safety.

> In fact it would be nice if the representation of &[u8] was stable and
> ffi-compatibe, I'd rather pass that directly to clearly express the intent.

That's almost certainly not going to be a thing for a while, as the rust developers want to be able to adjust the layout of types like slices in the future. I think this is your best bet for now.

> > I don't see why this is unsafe, the fields of this struct are private and should only be provided either from the adjacent `new` function or over FFI (which has to hold validity anyways). I think this should not be marked as unsafe for that reason. Perhaps it would be better in a submodule however, so that the code which uses it is unable to see the fields.
> 
> as_slice is totally not safe, I much prefer the unsafe block to be where we
> are taking the responsibility of insuring safety, which is in each ffi
> function where we use as_slice.

In what way is as_slice not safe? The only way to create one of these ByteSlice objects is either:
a) Through ByteSlice::new() - this type is trivially safe to convert back into a slice, as it was made from one!
b) When calling a _safe_ extern "C" function from C++.

In rust (and thus also when calling into rust code) the expectation of the caller is to ensure that all of the arguments meet all invariants before calling the function. With safe functions, those invariants are checked by the type system, while in unsafe functions those invariants are defined in comments and must be checked by the programmer.

When calling the functions which accept a ByteSlice, there is an invariant that the byte slice must contain a valid reference to some data, and the correct length, and that that data must outlive the call to the rust function. Thus, assuming that that invariant (which is maintained within rust and can only be violated by unsafe code or C++ code) is OK. I don't think that as_slice should be unsafe because saying it is is not a very useful statement.

We can talk more about what is safe and unsafe in rust code if you're interested.

> > In addition, I think this method should tie the lifetime of the output slice to the 'l lifetime parameter, as that is the lifetime of the underlying data.
> 
> I am not sure I totally grasp the exact difference between the two. My
> understanding is that as it currently is, the lifetime of the output slice
> is a subset of 'l, isn't this the desired behavior?

Currently the function has the elaborated type signature:

fn as_slice<'a, 'l>(this: &'a ByteSlice<'l>) -> &'a [u8];

In other words, it takes a reference to the ByteSlice object, and returns a slice of u8 which cannot outlive the lifetime of the ByteSlice. That being said, the ByteSlice object has a lifetime argument which represents the actual lifetime of the underlying byte buffer. As the ByteSlice object doesn't have to exist for the buffer to exist, it would be more useful to return a &'l [u8], as it could live longer in some situations. For example, consider this contrived function:

fn a() {
    extern "C" {
        // This function takes a ByteSlice and returns some slice into that ByteSlice which uses the same backing buffer based on some super cool logic implemented in C++!
        fn subslice<'a>(in: ByteSlice<'a>) -> ByteSlice<'a>;
    }
    let data = vec![1, 2, 3];
    let slice_1 = &data[..];
    let slice_2 = unsafe { subslice(ByteSlice::new(slice_1)) }.to_slice();
}

With the current implementation this would not compile, as slice_2 would have to outlive the ByteSlice object returned from subslice, but with the change to returning a &'l [u8] it would compile, which is correct as the slice only holds a reference into the original data vector.

If you have other questions let me know :).

Comment 10

[Nicolas Silva [:nical]]

(In reply to Michael Layzell [:mystor] from comment #9)
> &'l u8 is a safe rust type which carries a lot of meaning with it beyond
> just "a pointer to some data which contains at least one u8 object". It
> states that it is a non-exclusive, freezing (no other code can modify it)
> reference with a lifetime of 'l

Yes, that's my point I wanted this type to have the same semantics.

> which references exactly one u8 which is correctly aligned.

...but this is where the problem is I guess.

> In what way is as_slice not safe? The only way to create one of these
> ByteSlice objects is either:
> a) Through ByteSlice::new() - this type is trivially safe to convert back
> into a slice, as it was made from one!
> b) When calling a _safe_ extern "C" function from C++.

as_slice is unsafe in the same way as dereferencing a raw pointer is unsafe. The Compiler expects the returned slice to be valid but does not have a way to check that it is the case.

> 
> In rust (and thus also when calling into rust code) the expectation of the
> caller is to ensure that all of the arguments meet all invariants before
> calling the function. With safe functions, those invariants are checked by
> the type system, while in unsafe functions those invariants are defined in
> comments and must be checked by the programmer.

Yes, and it is the piece of code closest to what's responsible for the parameters to be valid that should have the unsafe block. In this case the ffi function typically looks like:

> pub extern "C" fn my_ffi_func(data: ByteSlice) {
>     // unsafe block here because it is the ffi boundary that
>     // is responsible for data to be in a valid state, and short
>     // of having a way to validate the input data, here is as
>     // close as we can be to the potential cause of the error.
>     let slice = unsafe { data.as_slice() };
>     // do stuff with slice

as_slice itself takes something unsafe (produced by C++ code, just like a raw pointer is unsafe) and adds no additional safety so it has no reason to be marked as safe and hide the unsafety.


> Thus, assuming that that invariant (which is maintained within
> rust and can only be violated by unsafe code or C++ code) is OK. I don't
> think that as_slice should be unsafe because saying it is is not a very
> useful statement.

I disagree. With the same reasoning any raw pointer produced by C++ through the ffi boundary should be safe to dereference or cast into a &-pointer, since one would argue assumption that invariants are respected beforehand.


> With the current implementation this would not compile, as slice_2 would
> have to outlive the ByteSlice object returned from subslice, but with the
> change to returning a &'l [u8] it would compile, which is correct as the
> slice only holds a reference into the original data vector.

Ah! right, thanks!

Comment 11

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nicolas Silva [:nical] from comment #10)
> Yes, that's my point I wanted this type to have the same semantics.

That's the idea behind the use of PhantomData<&'l [u8]> - what that means is basically "this struct has semantics as though it contains a &'l [u8], but I'm actually storing the required data in other unsafe forms (like *const u8, usize).

-----

Now let's talk about ByteSlice<'l> and safety :)

When C++ calls one of our extern "C" functions, we're depending on it not invalidating the invariants which are being held at the call boundary. The rust function is not checking (nor is there a way for it to) that the argument passed to it is in fact valid, no matter what it _must_ make the assumption that the argument passed to it is valid. Thus, we need to somehow state this invariant.

So, we have two implementation options:

A) Encode the invariant in the type.

The invariant is being held because we say that if C++ hands us a ByteSlice<'l>, then C++ is guaranteeing us that it is holding a valid reference to a byte slice which lives for lifetime 'l.

    impl ... {
        // ... 
        pub fn get(&self) -> &'l [u8] { unsafe { slice::from_raw_parts(...) } }
    }
    #[no_mangle]
    pub extern "C" fn use(s: ByteSlice) {
        let _ = s.get();
    }

The parallel to this with references would be writing an extern "C" function which took a non-nullable pointer to a byte as:

    #[no_mangle]
    pub extern "C" fn use(s: &u8) { ... }

Namely, the function is marked as safe, and the invariant required are encoded in the types being passed to the function.

B) Require the invariant at the call site.

In this situation the invariant is being held because the extern "C" function is unsafe, which signals to the caller that it has invariants beyond those stated in the type signature. In this case we would add a comment explaining that the invariant which is being required is that the ByteSlice object must contain a valid reference to a byte slice which lives for lifetime <'l>.

    impl ... {
        // ... 
        pub unsafe fn get(&self) -> &'l [u8] { slice::from_raw_parts(...) }
    }
    // INVARIANT: s must contain a ByteSlice object which is valid for the lifetime 'l
    #[no_mangle]
    pub unsafe extern "C" fn use(s: ByteSlice) {
        let _ = s.get();
    }

The parallel to this with references would be writing an extern "C" function which took a non-nullable pointer to a byte as:

    // INVARIANT: s must be a valid non-null pointer
    #[no_mangle]
    pub unsafe extern "C" fn use(s: *const u8) { ... }

Namely, the function is marked as unsafe, and the invariants required are encoded in the comments on the function, rather than in the type signature.

-----

The current implementation in this patch takes an unsafe approach, which is to state that both A) ByteSlice<'l> is not guaranteed to contain a valid &'l [u8], and B) the function is safe to call with any ByteSlice<'l>. This current implementation is definitely not correct, and one of the above solutions should be taken. The choice of which to choose is dependent on where we decide we feel the most comfortable placing the unsafe boundary.

I talked with Manishearth about where stylo has been choosing to draw the line. They have been choosing to follow the strategy described in A), for example, they have defined special rust types to represent reference counted gecko objects, which hold and encode those invariants, yet have repr(C) raw pointer internals. These types are safe to use in Rust, and the expectation is placed that C++ must guarantee their validity whenever passing to a function, rather than checking the specific function in question.

This is also the strategy taken by the nsstring rust bindings which I have written (they are at xpcom/rust/nsstring) - they add the types `nsACString` and `nsAString` which are assumed to be valid when passed into rust over FFI.

I have been suggesting, and continue to believe that, style A is the best style for consistency across gecko. However, style B is also OK. 

-----

Having this discussion about unsafe code styles has led me to believe that we need better community-accepted documentation about what unsafe means in rust, and where to draw unsafe boundaries, especially when interacting with FFI code. I'll look into starting this conversation in the rust community :).

Comment 12

[Nicolas Silva [:nical]]

I have a preference towards option B. And I realize that I would rather move the unsafe keyword in the function definition than in leave it in the function body (where it currently is), but not necessarily for the same reason.

It looks to me like the source of the debate is whether:
 - 1) Unsafe acts as a marker for code that can produce memory safety issues (in which case the ffi boundary is unsafe both ways). This would be where I stand.
 - 2) Unsafe is only a way to signal what code can be safely called from rust, but as long as it is C++ calling rust there is no notion of unsafety since C++ would be like an unsafe block of its own. This means we only consider the meaning of unsafe from the point of view of rust code calling into potentially unsafe code, not potentially unsafe code calling into rust (with potentially bad parameters).

We had a (way too long) debate about this with some servo folks today and some people assume (1) while others assume (2). It seems to me that all of my arguments take (1) for granted while yours take (2) for granted.

> The current implementation in this patch takes an unsafe approach, which is to state that both A) ByteSlice<'l> is not guaranteed to contain a valid &'l [u8], and B) the function is safe to call with any ByteSlice<'l>.
> This current implementation is definitely not correct, and one of the above solutions should be taken. The choice of which to choose is dependent on where we decide we feel the most comfortable placing the unsafe boundary.

The implementation in this patch only takes the approach of putting an unsafe pointer and a length in a struct instead of passing them separately, because it is easier to understand and more convenient. This is not any less safe or correct than the already existing code, so I'm going to assume you also consider the code without the patch to be definitely not correct.


The code as it exists today takes the approach of saying we dereference that pointer, because this function in particular trusts it to be correct, if it isn't the case this (the ffi boundary) is a good place to start investigating.
Since we interact with a system (Gecko) that isn't verified by the rust compiler and errors can come from either this system of the interaction with this system, there has to be (in my opinion) something marked unsafe around that interaction.

I believe that the most valuable place for this unsafe marker to be is in the code responsible for guaranteeing the safety (if possible) or the code that signals "I trust that it is safe". In the webrender bindings this code is the ffi function, not the convenience struct that bundles up a pointer and a length.


To be fair, following my argument would mean that we should not be passing the other arguments like api: &mut RenderApi as safe pointers, and its true and it itches me that any pointer coming from c++ can be safe. The types of these pointers on the C++ side are all forward declarations so the only way to get them wrong without trying to is to pass null, while the pointer and length thing tends to be more error prone, but granted it's not a real excuse. Following the other side of the argument, no parameter coming from C++ should be unsafe which doesn't make sense if you are in the group (1).

Comment 13

[Nicolas Silva [:nical]]

Anyway, I think we have spent more time debating this than it would have taken fixing the bugs we are trying to prevent, so I'll yield and reluctantly go for what the stylo folks are doing since that makes it the de-facto convention.

Comment 14

[Nicolas Silva [:nical]]

Attachment: Add BytSlice to pass const slices across ffi.

ByteSlice::as_slice is now assumed safe and ByteSlice doesn't have a lifetime annotation anymore because of a rustc bug which warns about PhantomData not being ffi-safe (and warning are treated as errors).

Comment 15

[Nicolas Silva [:nical]]

Attachment: Add MutBytSlice to pass mutable slices across ffi.

Ditto.

Comment 16

[Nicolas Silva [:nical]]

Attachment: Add the bindings glue for BlobImageRender

The renderer itself will be implemented in a followup patch.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8850965 [details] [diff] [review]
Add the bindings glue for BlobImageRender

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

::: gfx/webrender_bindings/src/bindings.rs
@@ +1360,5 @@
>      state.frame_builder.dl_builder.push_built_display_list(dl, aux);
>  }
> +
> +struct Moz2dImageRenderer {
> +    images: HashMap<ImageKey, BlobImageResult>

What's the purpose of this HashMap?

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8850963 [details] [diff] [review]
Add BytSlice to pass const slices across ffi.

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

Please fix to the commit message to be ByteSlice instead BytSlice.

Comment 19

[Nicolas Silva [:nical]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)
> What's the purpose of this HashMap?

It stores the rasterized images until, towards the end of the frame building they are queried by the resource cache. Right now it's quite naïve in the sense that all images are drawn on the render backend thread so we just put them there, but it'll get more complicated when we start running paint jobs in a thread pool.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/ffaa5c07c861
Add ByteSlice to pass constant u8 slices across ffi. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/3c44df6bb970
Add MutByteSlice to pass mutable u8 slices across ffi. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/3b0e181ef22d
Implement the BlobImageRenderer binding glue. r=jrmuizel

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/ffaa5c07c861
https://hg.mozilla.org/mozilla-central/rev/3c44df6bb970
https://hg.mozilla.org/mozilla-central/rev/3b0e181ef22d

Comment 22

[Nicolas Silva [:nical]]

Attachment: Implement a very naive BlobImageRenderer based on Moz2D Recording and Skia

Brace yourself it's quite ugly, but it seems to work (at least with simple drawing commands).

The next steps are:
 - Figure out a more efficient serialization. Right now everything including fonts and images is inlined in the recording and the serialized data gets copied around way too many times before it is run in the render backend.
 - Figure out how to make that work with tiled images.
 - Render in a thread pool instead of the render backend thread.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Are we still using this bug and this approach?

Comment 24

[Jeff Muizelaar [:jrmuizel]]

We're using this approach, but the work is done.