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

Support external image texture/buffer #547

Closed
wants to merge 2 commits into from

Conversation

JerryShih
Copy link
Contributor

@JerryShih JerryShih commented Nov 9, 2016

This patch try to support external image in WR (#524).

Then, we could use these two callback function to get/release the external buffer/image from c++.

pub get_func: GetExternalImageCallback
pub release_func: ReleaseExternalImageCallback

This change is Reviewable

@JerryShih
Copy link
Contributor Author

@nical @glennw
How about this for the external image problem?
Then, we could create a new display item type "ExternalImageItem" with the external image key. When the WR tries to render that ExternalImageItem item, WR could use the GetExternalImageCallback() to get the external buffer slice or gl_texture id for painting.

@JerryShih
Copy link
Contributor Author

@glennw
Could we have a default value for the struct's member?
Like:

pub struct RendererOptions {
     ....
     pub renderer_kind: RendererKind,
     pub external_image_callback: Option<ExternalImageCallback> = None,
 }

}

#[repr(C)]
pub struct ExternalImageStruct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

struct RgbImage {
    size: Size2D<u32>,
    stride: u32,
    format: ImageFormat,
    buffer: *const u8,
}

struct GlHandle {
     handle: u32,
}

// YuvImage, DxgiHandle, etc...

enum ExternalImage {
    RgbMemory(RgbImage),
    YuvMemory(YuvImage),
    Gl(GlHandle),
    Dxgi(DxgiHandle),
    // etc..
}

Using enums this way is a lot more idiomatic in rust, and I think that it'll make it easier to work with internally

@glennw
Copy link
Member

glennw commented Nov 10, 2016

Similarly to what @nical said, I think having the source data as a rust enum makes sense. If we do this, we won't even need to add a new public API for adding external images - we could change the last parameter of the existing add_image() function from taking a byte array to take a rust enum, something roughly like:

pub enum ExternalImageHandle {
    DXGI(dxgi_handle),
    GLTexture(i32),
    Raw(usize),
}

pub enum ImageData {
    Bytes(Vec<u8>),
    External(ExternalImageHandle),
}

fn add_image(&mut self, width: u32, height: u32, format: ImageFormat, data: ImageData) -> ImageKey;

And then WR would invoke the callback you've added when it encounters an image where the data is ImageData::External. The callback signature would take the ExternalImageHandle as a parameter.

Thoughts?

@JerryShih
Copy link
Contributor Author

@glennw @nical

If we have:

struct RgbImage {
    size: Size2D<u32>,
    stride: u32,
    format: ImageFormat,
    buffer: *const u8,
}

struct GlHandle {
     handle: u32,
}

// YuvImage, DxgiHandle, etc...

enum ExternalImage {
    RgbMemory(RgbImage),
    YuvMemory(YuvImage),
    Gl(GlHandle),
    Dxgi(DxgiHandle),
    // etc..
}

What's the corresponding c++ struct type for rust "repr(C)" ExternalImage enum type?
If we have that, the callback will become clear like:

// return a rust enum
pub type GetExternalImageCallback = fn(ExternalImageKey) -> ExternalImage

Otherwise, I will still need to have a big temporary struct ExternalImageStruct which contain all fields from all enum types and convert ExternalImageStruct to ExternalImage in binding code like (this PR already has this approach):

// get ExternalImageStruct struct instead of ExternalImage enum from c++
pub type GetExternalImageCallback = fn(ExternalImageKey) -> ExternalImageStruct

#[repr(C)]
pub struct ExternalImageStruct {
    image_type: ExternalImageType,

    // external texture handle for gl or dx
    handle: u32,
    target: u32, // 

    // rgb buffer or yuv buffer's size and format
    width: usize,
    height: usize,
    format: u32,

    // rgb or y channel
    buff0: *const u8,
    stride0: usize,
    size0: usize,

    // u channel
    buff1: *const u8,
    stride1: usize,
    size1: usize,

    //v channel
    buff2: *const u8,
    stride2: usize,
    size2: usize,
}

// convert ExternalImageStruct to ExternalImage enum
impl ExternalImageStruct {
    pub fn get_image(&self) -> ExternalImage {
        match self.image_type {
            ExternalImageType::GL_HANDLE =>
                ExternalImage::Gl {....},
            ExternalImageType::RGB =>
                ExternalImage::RgbMemory { .... },
            ExternalImageType::YUB =>
                ExternalImage::YuvMemory { .... },
            .........
        }
    }
}

@nical
Copy link
Contributor

nical commented Nov 10, 2016

@JerryShih and I talked about this offline and we think that as a first step it would be good to simplify this down to only exposing TextureId in the callbacks (we'd do the eventual texture uploads / dxgi glue, etc in gecko for now). This would simplify the callback interface into something that easy to express in both C++ and rust and avoid polluting the API with structures that would be better expressed as rust enums. Also, it would give us more time to figure out a more elaborate API to deal with the external image interface.

We should have a vidyo chat about this to figure out what servo and gecko need in the longer term.

@nical
Copy link
Contributor

nical commented Nov 10, 2016

@glennw I don't think we can / want to have the handles (GL/DXGI/etc.) stored in the display list. The thread that produces the display list does not know the value of this handle, which will be created on the compositor/renderer thread later. The idea behind ExternalImageKey is to have something that we can refer to (when creating the display list and in the render backend) before we create the actual handles (on the compositor/render thread). It also has the benefit of letting us change the texture handle associated to a key without rebuilding the frame if only the content of the current image has changed, which is pretty common when watching videos).

@bors-servo
Copy link
Contributor

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

@glennw
Copy link
Member

glennw commented Nov 17, 2016

@JerryShih @nical Should we close this PR? The basic external native texture support has landed now.

@JerryShih
Copy link
Contributor Author

Thanks. I'm trying to use that interface in Gecko.

@JerryShih JerryShih closed this Nov 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants