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

Remove image_data field from CanvasPixelData #13734

Closed
glennw opened this issue Oct 12, 2016 · 16 comments

Comments

Projects
None yet
4 participants
@glennw
Copy link
Member

commented Oct 12, 2016

This can be done once the removal of the old render system lands.

@jdm

This comment has been minimized.

Copy link
Member

commented Nov 4, 2016

Code: components/canvas_traits/lib.rs

@jdm jdm added this to the Swiss Hackathon milestone Nov 4, 2016

@dbrgn

This comment has been minimized.

Copy link

commented Nov 5, 2016

I'll give this a stab as part of the Swiss Hackathon :)

@jdm jdm added the C-assigned label Nov 5, 2016

@jdm

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

Great!

@dbrgn

This comment has been minimized.

Copy link

commented Nov 5, 2016

The tests seem to take ages to compile and run. Is there a way I can run a subset of the tests specific to this issue?

@jdm

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

If this builds, I would be fine with making a PR and running the tests on our build machines instead.

@dbrgn

This comment has been minimized.

Copy link

commented Nov 5, 2016

This is the current state, but now it complains about this:

error: no field `image_data` on type `canvas_traits::CanvasPixelData`
   --> /home/danilo/Projects/servo/components/script/dom/htmlcanvaselement.rs:210:35
    |
210 |                     => pixel_data.image_data.to_vec(),
    |                                   ^^^^^^^^^^ did you mean `image_key`?

error: aborting due to previous error

I'm not sure I can remove that as well...

@jdm

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

Hum. I believe we need to use the webrender API to retrieve the pixel data using the image key? @glennw Any suggestions how to do that?

@glennw

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2016

@jdm This is more a question for @emilio :)

@emilio

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

@jdm If it's completely needed, in the case of the webgl context you should be able to use the same path that readPixels (sending a command to WR). In the 2d canvas case it would need to use the draw target, there's code that does that also.

I'm not in front of my computer right now, I'll try to give more details when I am if needed :)

@emilio

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

But I believe the pixels in the 2d display item can go away with WR too.

@dbrgn

This comment has been minimized.

Copy link

commented Nov 5, 2016

The thing is that I don't really understand the code I was editing (I'm not very familiar with Canvas), so I'm not sure if I'm the right person to remove things from here :)

@emilio

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

So this ended up not being as trivial as I thought because we were using LayoutMsgs to get the pixels from script.

@emilio

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

@dbrgn Mind if I take this one?

@dbrgn

This comment has been minimized.

Copy link

commented Nov 6, 2016

@emilio sure, go on!

@jdm

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

Sorry @dbrgn! I assumed this would be less complex than it turned out to be.

@dbrgn

This comment has been minimized.

Copy link

commented Nov 6, 2016

No problem :) But at least I got to know the build process a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.