Skip to content

/ servo

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

webgl: Make a general way to get data from a JS array buffer view #8970

Merged
merged 4 commits into from Jan 5, 2016
+146 −65
Merged

webgl: Make a general way to get data from a JS array buffer view #8970

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4d32444
webgl: Unify accessing ArrayBufferView objects
emilio Jan 4, 2016
9ad49c8
conversion: Extrapolate array_buffer_view_data
emilio Jan 4, 2016
43d395a
conversions: Add a checked version to array_buffer_view_data
emilio Jan 4, 2016
d30f055
conversions: Make a `is_type_compatible` method for ArrayBufferViews.
emilio Jan 4, 2016
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next
Loading status checks…

webgl: Unify accessing ArrayBufferView objects 

This fixes an invalid length being reported from
float32_array_to_slice (which used the byte length), and also to
generalize getting data from a JS array buffer view, to reduce code
duplication.

The pending type safety issues, like where we could send a UInt16Array
where we expect a Float32 one, should be solved by IDL bindings in
some cases, like uniform[n]fv or vertexAttrib[n]fv, and with extra
checks in others, like in the pending texImage2D(..., ArrayBufferView).
  • Loading branch information
@emilio
emilio committed Jan 4, 2016
commit 4d32444bf731f75c29e6bad7777ae8eacb96bcdc
41 components/script/dom/webglrenderingcontext.rs
View file
Open in desktop
@@ -29,15 +29,15 @@ use dom::webgluniformlocation::WebGLUniformLocation;
use euclid::size::Size2D;
use ipc_channel::ipc::{self, IpcSender};
use js::jsapi::{JSContext, JSObject, RootedValue};
use js::jsapi::{JS_GetFloat32ArrayData, JS_GetObjectAsArrayBufferView};
use js::jsapi::{JS_GetObjectAsArrayBufferView};
use js::jsval::{BooleanValue, DoubleValue, Int32Value, JSVal, NullValue, UndefinedValue};
use net_traits::image::base::PixelFormat;
use net_traits::image_cache_task::ImageResponse;
use offscreen_gl_context::GLContextAttributes;
use script_traits::ScriptMsg as ConstellationMsg;
use std::cell::Cell;
use std::sync::mpsc::channel;
use std::{ptr, slice};
use std::{ptr, slice, mem};
use util::str::DOMString;
use util::vec::byte_swap;

@@ -167,31 +167,16 @@ impl WebGLRenderingContext {
}

#[allow(unsafe_code)]
fn float32_array_to_slice(&self, data: *mut JSObject) -> Option<Vec<f32>> {
fn array_buffer_view_to_vec<T: Clone>(&self, data: *mut JSObject) -> Option<Vec<T>> {

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin Jan 4, 2016

Member

Having T unrestricted (other than T: Clone) seems dangerous. There’s nothing stopping a user from calling this with T = String, causing .to_vec() to interpret arbitrary buffer data as a pointer and dereference it.

So either this function needs to be unsafe, or an unsafe trait should be used:

unsafe trait ArrayBufferContents: Clone {}
unsafe impl ArrayBufferContents for u8 {}
unsafe impl ArrayBufferContents for i8 {}
unsafe impl ArrayBufferContents for u16 {}
unsafe impl ArrayBufferContents for i16 {}
unsafe impl ArrayBufferContents for u32 {}
unsafe impl ArrayBufferContents for i32 {}
unsafe impl ArrayBufferContents for f32 {}
unsafe impl ArrayBufferContents for f64 {}
fn array_buffer_view_to_vec<T: ArrayBufferContents>(&self, data: *mut JSObject) -> Option<Vec<T>> {

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin Jan 4, 2016

Member

While you’re at it, is the .to_vec() copy necessary? Or could this return Option<&[T]>? How long is the pointer from JS_GetObjectAsArrayBufferView valid?

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 4, 2016

Author Member

The to_vec() is necessary in the context of WebGL since it's going to be sent over to the WebGL task, but not needed otherwise.

I updated the PR, and used it in the other sites where JS_GetObjectAsArrayBufferView was being called.
unsafe {
let mut length = 0;
let mut byte_length = 0;
let mut ptr = ptr::null_mut();
let buffer_data = JS_GetObjectAsArrayBufferView(data, &mut length, &mut ptr);
let buffer_data = JS_GetObjectAsArrayBufferView(data, &mut byte_length, &mut ptr);
if buffer_data.is_null() {
self.webgl_error(InvalidValue); // https://github.com/servo/servo/issues/5014
return None;
}
let data_f32 = JS_GetFloat32ArrayData(data, ptr::null());
Some(slice::from_raw_parts(data_f32, length as usize).to_vec())
}
}

#[allow(unsafe_code)]
fn byte_array_to_slice(&self, data: *mut JSObject) -> Option<Vec<u8>> {
unsafe {
let mut length = 0;
let mut ptr = ptr::null_mut();
let buffer_data = JS_GetObjectAsArrayBufferView(data, &mut length, &mut ptr);
if buffer_data.is_null() {
self.webgl_error(InvalidValue); // https://github.com/servo/servo/issues/5014
return None;
}
Some(slice::from_raw_parts(ptr, length as usize).to_vec())
Some(slice::from_raw_parts(ptr as *mut T, byte_length as usize / mem::size_of::<T>()).to_vec())
}
}

@@ -471,7 +456,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
Some(data) => data,
None => return self.webgl_error(InvalidValue),
};
if let Some(data_vec) = self.byte_array_to_slice(data) {
if let Some(data_vec) = self.array_buffer_view_to_vec::<u8>(data) {
handle_potential_webgl_error!(self, bound_buffer.buffer_data(target, &data_vec, usage));
}
}
@@ -495,7 +480,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
if offset < 0 {
return self.webgl_error(InvalidValue);
}
if let Some(data_vec) = self.byte_array_to_slice(data) {
if let Some(data_vec) = self.array_buffer_view_to_vec::<u8>(data) {
if (offset as usize) + data_vec.len() > bound_buffer.capacity() {
return self.webgl_error(InvalidValue);
}
@@ -973,7 +958,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
None => return,
};

if let Some(data_vec) = self.float32_array_to_slice(data) {
if let Some(data_vec) = self.array_buffer_view_to_vec::<f32>(data) {
if data_vec.len() < 4 {
return self.webgl_error(InvalidOperation);
}
@@ -999,7 +984,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
#[allow(unsafe_code)]
// https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
fn VertexAttrib1fv(&self, _cx: *mut JSContext, indx: u32, data: *mut JSObject) {
if let Some(data_vec) = self.float32_array_to_slice(data) {
if let Some(data_vec) = self.array_buffer_view_to_vec::<f32>(data) {
if data_vec.len() < 4 {
return self.webgl_error(InvalidOperation);
}
@@ -1015,7 +1000,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
#[allow(unsafe_code)]
// https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
fn VertexAttrib2fv(&self, _cx: *mut JSContext, indx: u32, data: *mut JSObject) {
if let Some(data_vec) = self.float32_array_to_slice(data) {
if let Some(data_vec) = self.array_buffer_view_to_vec::<f32>(data) {
if data_vec.len() < 2 {
return self.webgl_error(InvalidOperation);
}
@@ -1031,7 +1016,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
#[allow(unsafe_code)]
// https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
fn VertexAttrib3fv(&self, _cx: *mut JSContext, indx: u32, data: *mut JSObject) {
if let Some(data_vec) = self.float32_array_to_slice(data) {
if let Some(data_vec) = self.array_buffer_view_to_vec::<f32>(data) {
if data_vec.len() < 3 {
return self.webgl_error(InvalidOperation);
}
@@ -1047,7 +1032,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
#[allow(unsafe_code)]
// https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
fn VertexAttrib4fv(&self, _cx: *mut JSContext, indx: u32, data: *mut JSObject) {
if let Some(data_vec) = self.float32_array_to_slice(data) {
if let Some(data_vec) = self.array_buffer_view_to_vec::<f32>(data) {
if data_vec.len() < 4 {
return self.webgl_error(InvalidOperation);
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.