Added Uniform{2i, 2iv, 2fv, 3f, 3i, 3iv, 3fv}

[autrilla]

@emilio r?

Partial #10417.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl
• @emilio: components/script/dom/webglrenderingcontext.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[Manishearth]

if the passed location is null, data passed in will be silently ignored.

I assume that means that we should return early here and not throw any errors?

[emilio]

That refers to the WebGLUniformLocation variable, which is ignored if it's null in Uniform2f()... That check should be repeated here though, since in the case null, null were registering an error when we should not.

[autrilla]

I suppose so, yes. The other previously implemented uniform vector functions didn't do it either, so should I fix that in this PR, or open an issue and handle it in another PR?

[emilio]

Preferably fixing it here, but if you don't have the time or anything feel free to file an issue :)

[Manishearth]

I don't see how this works. This will only work for glUniform2fv(uniform, 1, data), i.e. uniform vec2 foo or uniform vec2 foo[1], but not for uniform vec2 foo[10], which is set as glUniform2fv(uniform, 10, data). In this case I'm assuming the WebGL API internally figures out the size because hey, we're not in C++-land anymore, but the behavior should be the same. We'll need a new message for it.

@emilio does this sound correct?

[emilio]

Yep, that's true... We also do this in other parts of the code, so we should add Uniformxy messages that receive a Vec<y> and allow that.

If you want to do it for this PR, that would be just great, but if you'd prefer to land it as is and leaving it as a followup it'd be fine too.

[bors-servo]

☔ The latest upstream changes (presumably #10432) made this pull request unmergeable. Please resolve the merge conflicts.

[autrilla]

@emilio could you review this changes to see if the general principle is fine? (Don't accept the PR or anything though)

[emilio]

@autrilla: Yeah those changes look in the good direction :)

[autrilla]

This should do it. I still want to refactor it a bit because it is too copy-pasty (maybe with a macro?). Extracting parts of the function is a bit hard because errors are expected to be returned directly from the function and not sent to the ipc_renderer... If anyone has any ideas on doing this cleanly, I'd appreciate them!

This should be covered by tests/wpt/web-platform-tests/webgl/conformance-1.0.3/conformance/more/functions/uniformi.html, although not extremely thoroughly.

[emilio]

Ok, so I'd try to abstract all this in a function belonging to WebGLRenderingContext and returning either true or false (true if it's valid, false if not).

The key point here, is that the return self.webgl_error(..) is actually a cool idiom, but it doesn't return nothing, so it's strictly equivalent to:

self.webgl_error(..);
return;

So... It isn't hard to imagine a function like:

fn validate_uniform_parameters<T>(&self, uniform: Option<&WebGLUniformLocation>, expected_size: u32, data: Option<&[T]>) -> bool {
    let uniform = match uniform {
        Some(uniform) => uniform,
        None => return, // No error, as spec'd
    };

    let data = match data {
        Some(data) => data,
        None => {
            self.webgl_error(InvalidOperation);
            return false;
        },
    };

    if data.len() % expected_size != 0 {
        // ...
    }

    // ...
    true
}

That way it can be reused both in the vector and not vector function, with just an extra check on data if we use optional object in the WebIDL:

fn Uniform2i(&self, uniform: Option<&WebGLUniformLocation>, x: i32, y: i32) {
    if !self.validate_uniform_parameters(uniform, 2, Some(&[x, y])) {
        return; // Error handled above
    }
    self.ipc_renderer...
}

fn Uniform2iv(&self, uniform: Option<&WebGLUniformLocation>, data: Option<*mut JSObject>) {
    let data_slice = data.and_then(|data| unsafe {
        array_buffer_view_data_checked::<i32>(data)
    });

    if !self.validate_uniform_parameters(uniform, 2, data_slice) {
        return; // Error handled above
    }
    self.ipc_renderer...
}

Would something like that make sense?

---

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[autrilla]

@emilio yes, that makes sense. Not needing to return self.webgl error definitely makes things easier. I'll update the code and push it again

[emilio]

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

components/script/dom/webglrenderingcontext.rs, line 195 [r7] (raw file):
It's probably better to pass directly &WebGLUniformLocation and the current program to the caller instead of unwrapping.

---

components/script/dom/webglrenderingcontext.rs, line 216 [r7] (raw file):
So, I'm not too confident this is the right way to check this, nor the most performant way.

We know what the expected type should be, eg (Uniform1i should have a type of constants::INT). We should pass that type to this function instead of using TypeId here.

Also, why do you check for data.len() >= 1 here and data.len() == 1 in constants::INT? It seems we should check that the length is a multiple of the expected one, that is: data.len() % n == 0.

Even more, why splitting this and validate_uniform_vector_parameters? The only check that could potentially fail is the size check, and this is only called from Uniformxy, where we actually know that size is right (because we're just faking the slice).

We should unify both functions, even when there are a few boilerplate arguments in the non-vector functions.

---

components/script/dom/webglrenderingcontext.rs, line 240 [r7] (raw file):
Same here about the unwrap()s

---

Comments from Reviewable

[autrilla]

components/script/dom/webglrenderingcontext.rs, line 216 [r7] (raw file):
The == 1 for constants::INT was a mistake. The length does not have to be a multiple of the expected length here, it has to be at least that size. This is not used for filling up arrays, it's used for filling up single webgl vectors. If we have an INT_VEC4, it makes as much sense to pass 5 parameters as it makes to pass 8. Really, anything beyond 4 makes no sense, but the spec does not say anything about supplying more values than actually fit in the uniform. In the end OpenGL is C, which does not really know about the actual size of the "array" (it's just a pointer) being passed, so you just get the first 4 values and keep going.

I think you're right on the rest.

---

Comments from Reviewable

[autrilla]

components/script/dom/webglrenderingcontext.rs, line 263 [r7] (raw file):
This is broken too. Say we're writing to an INT_VEC3 array of two elements, and we pass [1, 2, 3, 4, 5, 6]. This would be totally valid, but I'm checking the length of the data, when I should be checking the length of the data divided by the size of each 'element'.

---

Comments from Reviewable

[bors-servo]

☔ The latest upstream changes (presumably #10565) made this pull request unmergeable. Please resolve the merge conflicts.

[autrilla]

@emilio review this whenever you get a chance, please

[emilio]

So much cleaner! Great job! :)

Left a few comments, mostly nits. This is ready to merge once they're adressed and the commits are squashed :P

---

Reviewed 5 of 6 files at r8.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

components/script/dom/webglrenderingcontext.rs, line 82 [r8] (raw file):
nit: Maybe element_count is more descriptive?

---

components/script/dom/webglrenderingcontext.rs, line 95 [r8] (raw file):
nit: maybe as_gl_enum or as_gl_constant is more descriptive?

---

components/script/dom/webglrenderingcontext.rs, line 222 [r8] (raw file):
Could you add a link to https://www.khronos.org/opengles/sdk/docs/man/xhtml/glUniform.xml (and potentially https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10)?

Ideally each block that could return false should be annotated with the relevant part of the spec, but if you drop the link here I won't complain :P

---

components/script/dom/webglrenderingcontext.rs, line 226 [r8] (raw file):
To clarify the above comment, for example, a comment like:

// If the passed location is null, the data passed in will be silently ignored
// and no uniform variables will be changed.

above this line would sufice :)

---

components/script/dom/webglrenderingcontext.rs, line 232 [r8] (raw file):
nit: maybe merge those two lines? I don't find match self.current_program.get() specially difficult to read.

---

components/script/dom/webglrenderingcontext.rs, line 248 [r8] (raw file):
Please add a TODO comment here with something like "dont't request this every time, cache it"

---

components/script/dom/webglrenderingcontext.rs, line 1133 [r8] (raw file):
nit: This is not very important. array_buffer_view_data_checked avoids copying the whole array in the case it's invalid, and wouldn't require the .as_ref().map(Vec::as_slice) below.

Not too important, since we should copy it in the common case where the slice is valid, so I won't complain if you leave this as-is :)

If you prefer to leave this as-is, please change the name to either data_vec or just data.

---

components/script/dom/webglrenderingcontext.rs, line 1147 [r8] (raw file):
This looks way cleaner, doesn't it? :)

---

Comments from Reviewable

[emilio]

r=me with that typo corrected :P

---

Review status: 4 of 5 files reviewed at latest revision, 10 unresolved discussions.

---

components/script/dom/webglrenderingcontext.rs, line 237 [r9] (raw file):
I meant to quote the spec literally, but it's fine, with the link it's not difficult to follow.

---

components/script/dom/webglrenderingcontext.rs, line 1148 [r9] (raw file):
nit: This should be array_buffer_view_to_vec::<i32>()

---

Comments from Reviewable

[autrilla]

r+? @emilio

[KiChjang]

@bors-servo r=emilio

[bors-servo]

📌 Commit 89c432b has been approved by emilio

[bors-servo]

⌛ Testing commit 89c432b with merge 7845d67...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt