Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite WebGLRenderingContext to use typed array APIs #14676

Closed
jdm opened this issue Dec 22, 2016 · 13 comments
Closed

Rewrite WebGLRenderingContext to use typed array APIs #14676

jdm opened this issue Dec 22, 2016 · 13 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Dec 22, 2016

We need to replace uses of array_buffer_view_data_checked, array_buffer_view_data, array_buffer_view_to_vec_checked, array_buffer_view_to_vec, and array_buffer_to_vec. This may mean adding an additional ArrayBufferViewU16 type to the list in rust-mozjs, but the rest should be straightforward replacements with things like ArrayBuffer::from and ArrayBufferView::from (after servo/rust-mozjs#323 merges).

Code: components/script/dom/webglrenderingcontext.rs
Tests: ./mach test-wpt tests/wpt/web-platform-tests/webgl/

@yysung1123
Copy link
Contributor

@yysung1123 yysung1123 commented Dec 29, 2016

I would like to work on this issue!

@yysung1123
Copy link
Contributor

@yysung1123 yysung1123 commented Dec 29, 2016

There is no JSContext in the fn validate_tex_image_2d_data which is required by ArrayBufferView::From.
How can I fix this problem? thx

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Dec 29, 2016

Does self.global().get_cx() work?

@jdm
Copy link
Member Author

@jdm jdm commented Dec 29, 2016

You can add a *mut JSContext argument to the method, since all of the callers should have access to one already.

@jdm jdm added the C-assigned label Dec 29, 2016
@yysung1123
Copy link
Contributor

@yysung1123 yysung1123 commented Jan 22, 2017

Sorry for that I was busy before.
I can work for it now!
It seems that there should be type ArrayBufferViewU16 in the rust/mozjs
Could you please create an issue on rust/mozjs for this? Thanks

@yysung1123
Copy link
Contributor

@yysung1123 yysung1123 commented Jan 23, 2017

Change
code

} else if array_buffer_view_data_checked::<u8>(data).is_some(){
    1
}

To

typedarray!(in(cx) let typedarray_u8: ArrayBufferView = data);
// some line
} else if typedarray_u8.is_ok() {
    1
}

Is it right?

@jdm
Copy link
Member Author

@jdm jdm commented Jan 23, 2017

That looks right.

@yysung1123
Copy link
Contributor

@yysung1123 yysung1123 commented Jan 23, 2017

@jdm

So I need ArrayBufferViewU16 in rust/mozjs to replace the function array_buffer_view_data_checked::<u16>.

But I don't know how to add this to rust/mozjs.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 23, 2017

You can add it to the list of typed arrays at https://github.com/servo/rust-mozjs/blob/master/src/typedarray.rs#L272, and then use a Cargo override to use your locally-modified version inside Servo.

@yysung1123
Copy link
Contributor

@yysung1123 yysung1123 commented Jan 24, 2017

It seems that I need to write C++ functions which are binding with two Rust functions UnwrapUint16ArrayBufferView and GetUint16ArrayBufferViewLengthAndData.

Where can I implement these function?

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jan 24, 2017

It look like we shouldn't be using ArrayBufferView for this at all, just Uint8Array and Uint16Array.

@yysung1123
Copy link
Contributor

@yysung1123 yysung1123 commented Jan 25, 2017

The trait bound in ArrayBufferViewContents is u8, u16, i8, i16, i32, f16, f32 primitive type.

It means that there would be some function calls like

typed_array_or_sequence_to_vec::<i32>(cx, data, ConversionBehavior::Default)

But I have to change them to Uint8, Uint16, Int8, Int16, Int32, Float16, Float32 which are TypedArrayElementType.

Therefore I can rewrite fn typed_array_or_sequence_to_vec with TypedArray.

@yysung1123
Copy link
Contributor

@yysung1123 yysung1123 commented Jan 27, 2017

TypedArray only uses TypedArrayElement type to be created.

But the generic type parameters of typed_array_or_sequence_to_vec:: are primitive type now.

How can I use primitive type to create an TypedArray? thx

bors-servo added a commit that referenced this issue Feb 2, 2017
…-array-APIs, r=jdm,emilio

Rewrite WebGLRenderingContext to use typed array APIs

Rewrite WebGLRenderingContext to use typed array APIs. Based on #15267.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14676
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15352)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Feb 10, 2017
…-array-APIs, r=jdm

Rewrite WebGLRenderingContext to use typed array APIs

Rewrite WebGLRenderingContext to use typed array APIs. Based on #15267.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14676
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15352)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.