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

Use Skia Canvas instead of Surface where possible #188

Merged
merged 6 commits into from Nov 21, 2019

Conversation

therealbnut
Copy link
Contributor

@therealbnut therealbnut commented Nov 20, 2019

Hey, I've made some changes to allow you to use Skia's Canvas wherever possible. This allows you to use resvg with PDFs 🎉.

https://skia.org/user/api/skcanvas_creation

The SkPDF backend uses SkDocument instead of SkSurface, since a document must include multiple pages.

Something like this:

let pdf_doc = skia::PDF::make_document(&pdf_stream);
let pdf_canvas = pdf_doc.begin_page(width, height);
resvg::backend_skia::render_to_canvas(&tree, &opt, size, pdf_canvas);

@therealbnut therealbnut changed the title Use Canvas instead of Surface where possible Use Skia Canvas instead of Surface where possible Nov 20, 2019
@RazrFalcon
Copy link
Owner

Well, it will create a pretty horrible PDF. There are plans on making a proper PDF target, but it will not be implemented any time soon.

As for the patch, C-API should be updated too. It requires Surface right now.

@RazrFalcon
Copy link
Owner

I've added a check for Skia C-API build. Turns out it was missing.

@therealbnut
Copy link
Contributor Author

therealbnut commented Nov 21, 2019

Well, it will create a pretty horrible PDF.

We're using it to render just graphics. Text, links and other things we do directly with Skia. It works really well, thank you for the awesome library!

I've just pushed a commit to update the Skia C-API.

I don't have a machine that can run ./testing-tools/run-tests.py, hopefully travis doesn't run into any issues :)

capi/include/resvg.h Show resolved Hide resolved
@RazrFalcon
Copy link
Owner

We're using it to render just graphics.

I see. As long as you are fine with results - it's ok. I just wanted to point out that it's quite limited. Especially when you have text and layers. But as long as you only have shapes without opacity - you should be fine.

@therealbnut
Copy link
Contributor Author

therealbnut commented Nov 21, 2019

I just wanted to point out that it's quite limited. Especially when you have text and layers.

Thanks for the info! We don't have text, although perhaps we will run into opacity, I guess if we can detect that we might rasterise those cases.

@RazrFalcon
Copy link
Owner

I think that build will fail, because there are no skia::Canvas::from_ptr. You should add

     pub unsafe fn from_ptr(ptr: *mut ffi::skiac_canvas) -> Option<Canvas> {
        if ptr.is_null() {
            None
        } else {
            Some(Canvas(ptr))
        }
    }

to the Canvas implementation in bindings/resvg-skia/src/lib.rs

@RazrFalcon
Copy link
Owner

@JaFenix Hi! Is this change fine for you? resvg_skia_render_to_canvas will accept a Canvas and not a Surface from now.

@therealbnut
Copy link
Contributor Author

therealbnut commented Nov 21, 2019

pub unsafe fn from_ptr(ptr: *mut ffi::skiac_canvas) -> Option {

Ah, good catch, thanks. I didn't realise I had to cd into capi/ when running cargo build to build it too, I thought it was part of the workspace.

@therealbnut
Copy link
Contributor Author

@RazrFalcon what do you think about adding something like this:

typedef struct sk_canvas_t sk_canvas_t;

(matching their offical C API https://github.com/google/skia/blob/master/include/c/sk_types.h#L153)

 void resvg_skia_render_to_canvas(const resvg_render_tree *tree,
                                   const resvg_options *opt,
                                   resvg_size size,
-                                  void *canvas);
+                                  skia_canvas_t * canvas);

So at the call site you might know at compile time about a type mismatch.

@RazrFalcon
Copy link
Owner

It's C. The are not compile time guaranties. And you would call resvg via C++ anyway, so there is no difference.

@therealbnut
Copy link
Contributor Author

Sorry about the failed builds. I'm having trouble running the tests locally, or building anything except the main package. Do they only work on linux?

@RazrFalcon
Copy link
Owner

No problem.

No, they should work on any platform.

@therealbnut
Copy link
Contributor Author

Awesome, thanks for reviewing 🎉

I suppose we can merge it once @JaFenix gives the okay?

@RazrFalcon
Copy link
Owner

Yes, he added the Skia backend initially and I do not want to break it for him.

@JaFenix
Copy link
Contributor

JaFenix commented Nov 21, 2019

Hi, I don't think this will break anything, because the surface instance that's passed in doesn't seem to be used for anything, other than to deliver a canvas. Changing our C++ caller to pass in the canvas won't be a problem. OK to merge 👌

@RazrFalcon
Copy link
Owner

Thanks for a quick response.

@RazrFalcon RazrFalcon merged commit 2e56929 into RazrFalcon:master Nov 21, 2019
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.

None yet

3 participants