-
Notifications
You must be signed in to change notification settings - Fork 12
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
Cairo renderer #7
Conversation
Thanks ! Will have a look at all this when I get some time.
Could you rather use W700 for this ? This would make it consistent with CSS, see Best, Daniel
|
{- [resolution], specifies the rendering resolution in samples per | ||
meters. The PDF, PS and SVG formats are measured in points by Cairo, | ||
while the PNG format is in pixels. If unspecified, the default | ||
conversion to points is used.}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is hard to understand. Is resolution that [resolution] argument expressed in samples per meters or not ? (P.S. do not clarify yourself I'm currently merging in another branch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides resolution only makes sense for raster formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that this comment is undecipherable and that the resolution argument should only be specified for the PNG backend. The second part tries to warn that the resolution for PNG isn't correct with the default value since we don't have access to the DPI. Also, since the resolution effectively acts as a scaling factor, I was trying to say "it scales from Vg units to Cairo units, but those differs depending on the backend, so you can just forget the physical units if you want a PNG with a specific size in pixels at the end".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get it but I have the impression that this API is wrong. It's not that you need to access the DPI, you should be able to set the DPI for the output image. Is there any way to set the physical size of the render target in cairo ? I see that the generated PNGs have no pHYs
chunks.
For the other backends wouldn't make it sense to simply ignore the argument ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think so. Cairo defines the units used for each of its backend, but doesn't care about the physical size otherwise (so pixel surfaces don't have a "printed real life size" metadata..)
The other backends effectively do not need to respect the resolution since the conversion from meters to points doesn't need to be configured; it seemed simpler to just use the default value and let users tweak the ratio it if they desire. I think Png of resolution
might be better than silently ignoring the argument if you care about enforcing the physical ratio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seemed simpler to just use the default value and let users tweak the ratio it if they desire.
I'd rather not mutiply the API points were ratios can be tweaked and I don't like the fact that the name of the argument doesn't reflect the semantics, so I'm going to ignore it for the other backends and simply use the point to meter conversion internally for those.
According to what I read for PNG this still represents the number of samples per meters right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it copies the handling of the resolution
argument for the js canvas.
{- [size], Surfaces created with [Cairo.Surface] have a valid size, while | ||
file based surfaces have a size of zero by default: If the size of the | ||
surface can not be determined, the optional argument [size] is used | ||
instead. [Invalid_argument] is raised if the size is invalid.}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the renderable size instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't resize a cairo surface, so I followed the semantics of the js canvas target : fill the surface and ignore the physical ratio. The size
optional argument is a bugfix for cairo api, because even though cairo won't tell us its value, the user still specified a size when he created it. It didn't seem right to default to something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't resize a cairo surface, so I followed the semantics of the js canvas target : fill the surface and ignore the physical ratio.
Yes this is ok.
The size optional argument is a bugfix for cairo api, because even though cairo won't tell us its value, the user still specified a size when he created it. It didn't seem right to default to something else.
Ok I didn't get that. I thought you could specify the size after the fact. Now what I don't like with this is that it can be error prone to switch from a memory based to file based surfaces. Should we maybe always force the user (re)specify the surface size. Or is there maybe a way to fix that so that the file based surfaces report their size correctly ?
More precisely could tell me how these zero-based sized surfaces get created ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, sure. The cairo bindings provides five modules for creating surfaces: the generic Surface, and the file-based PDF, PS, SVG and PNG. The first one, Surface, creates and manages the surfaces in memory and is the motivation for the function target_surface
since it provides a lot of non-file formats (and you could just use the simpler target
if you wanted to dump the result in a file.) The function to create a new surface has the type format -> width:int -> height:int -> surface
. While there are no resize capability, such a surface reports the correct size when asked (so the ?size
argument is ignored.)
The four remaining modules allow you to create a file-based surface. For unknown reasons, this surfaces report a size of (0,0)
when asked (so the ?size
argument is used.) The PDF and PS surfaces do support resizing (but I think it would be confusing to arbitrarily resize for them.)
I expect people to use target_surface
with surfaces created by the module Surface, typically for rendering textures that are then displayed directly to the screen with opengl or whatever. Very exceptionally, someone might want to use target_surface
with one of the file-based module so that he can configure lowlevel stuff... but then we need to know the image size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PDF and PS surfaces do support resizing (but I think it would be confusing to arbitrarily resize for them.)
Shouldn't we rather resize according to the renderable's size ? That's what for example vg's built-in PDF renderer does.
The four remaining modules allow you to create a file-based surface. For unknown reasons, this surfaces report a size of (0,0) when asked (so the ?size argument is used.)
If this is a bug I'm somewhat reluctant to adapt the API for the bug...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a bug I'm somewhat reluctant to adapt the API for the bug...
So this is not a bug per se. The fact is that each kind of surface has its own functions and you can't use functions for a surface of a given kind with the other.
After a long discussion with @Chris00 we came to the conclusion that this is not a good interface and that it is preferable to operate directly on contexts rather than surfaces (e.g. this will allow to use Cairo_gtk which is not possible at the moment).
So we forget about the size
argument for the target. The semantics is then that given a renderable (size, view, i)
we setup a transform so that view
rectangle is mapped on the rectangle Box2.v P2.o size
in the coordinate system as setup in the given context when we do the render operation. This means that if you create your surface with size
, get the context, create a target with it and your renderable is (size, view, i)
it will do the expected, that is map the view
rectangle on the whole surface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, yes, this sounds good. Thanks for clearing it up with Chris00!
Ok cool. I was suspecting that.
I'm using cairo on on osx. That may explain some things. I'll try to have a closer look at it. |
I don't have a clue, but I only tested in on linux since I don't have access to anything else. I'll let you know if I think of some other explanation. You can also send me the rendering by mail if the bug isn't always "the texts are 50x larger than they should", it could help. Otherwise, the outputs I have are identical to the pdf backend (except for the gradients which are subtly not computed in the correct colorspace, but that's all.) |
Yes that's the symptom. I don't think the problem is with your code. It seems rather to be a bug in cairo. See graphite-project/graphite-web#1191 (comment). |
let c = V2.((q + 2. * p0) / 3.) in | ||
let c' = V2.((pt + 2. * q) / 3.) in | ||
P2.(Cairo.curve_to s.ctx (x c) (y c) (x c') (y c') (x pt) (y pt)); | ||
loop pt segs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. @Chris00 I was trying to support multiple page output and it seems I get the following assertion failure:
Any idea ? |
Never mind can no longer reproduce... |
Ok so your PR has been squashed and merged as bbcabd4 I changed a bit the API and made stored renderer for pdf and ps support multiple page output. Thanks for bootstrapping this and sorry again for the long delay. Release coming soon. |
Great! Thanks for taking the time to make so many fixes, I'm very happy with all the improvements you made. Have a nice day! |
The main motivation for this backend is the ability to rasterize a Vg image to a custom surface in memory, but it can also be used to produce PNG and PS files (and more redundantly, PDF and SVG.) It adds an optional dependency to the
cairo2
bindings for OCaml.The implementation is based on the existing Vgr_htmlc renderer, as the semantics of the javascript canvas are very close to the cairo surface model, with the following known limitations:
W600
.)The
rcairo
demo has an additional-format
argument to specify the file backend.Let me know if I overlooked anything!