Hi!
On Mon, Jul 2, 2012 at 4:47 AM, Carlos López González
<genet...@gmail.com> wrote:
> Hi!
>
> 2012/7/2 Nikita Kitaev <nikita...@gmail.com>:
>> Nice, the code's coming together! I played around with it today and
>> fixed a couple of compile errors.
>>
>
> Oh, I don't have compile errors on the latest committed version, can I
> have some details of what was missing/wrong?
I think my commit details everything. I added a forward declaration of
CairoSurface. Also, on my machine png.h must be imported before any of
the cairo headers.
There's a chance that some of my own changes made these errors show themselves.
One more subtle bug: the constructor "CairoSurface(cairo_surface_t
*cs) { set_cairo_surface(cs); }" needs to initialize ":cs_(NULL)".
Otherwise set_cairo_surface() calls cairo_surface_destroy(cs_) on an
uninitialized pointer.
>> The structure feels good, I just have a few suggestions:
>> 1) start_frame() and end_frame() feel redundant.
>>
>> The purpose they serve in the scanline target is to signal the start
>> and end of data transfer from the Surface onto the underlying target.
>> The reason they're two separate methods is that in between,
>> start_scanline() and end_scanline() are called repeatedly. The cairo
>> renderer doesn't operated on scanlines, so I suggest a new structure:
>>
>> -- obtain_surface() does all of the initialization related to the
>> beginning of a new frame, and returns either a surface or NULL, in
>> case of failure.
>
> Since I didn't have all the details of the particular backends I
> preferred to leave it there.
> For instance, start_frame() can be used to test whether the surface is
> still valid or have reached a non valid status (temporarily non
> writable device for example) so I think that the target has to be
> asked to perform a drawing operation that might depends on the
> specific backend.
Can you clarify? Why would the target need to perform a drawing
operation? Presumably it can just ask the device if it is ready.
>
>> -- add_frame() takes the finished surface, transfers the data wherever
>> it needs to go, and destroys the surface
>>
>> 2) obtain_surface() should return cairo_surface_t* instead of
>> CairoSurface. The targets don't need the etl::surface API. It feels
>> redundant to have them wrap a cairo_surface_t* in a CairoSurface at
>> the start of the render, extract the cairo_surface_t* at the end of
>> the render, and not use any of the CairoSurface methods.
>>
>
> obtain_surface would be used in two circumstances:
> 1) When the root Target needs to ask the derivative Target to create a
> new surface with the current RendDesc data. In this case it should
> return a CairoSurface because it will be passed to the
> accelerated_render structure which in some cases would need to map and
> unmap the surface to a image surface for direct modification.
> 2) In the middle of the Layer's render procedure there is need to
> create a cairo image surface to make temporarly renders using direct
> access to memory. Since that intermediate surface is going to be
> merged with the current surface being rendered, it should be a
> cairo_image_surface type so I need to create one new CairoSurface from
> the Cairo Image backend. It will be performed by the same way that
> Layer::accelerated_render does: etl::handle<Target>
> target=cairotarget2surface(Surface). I just need to add a new function
> to create a special cairotarget2surface that either accept a RendDesc
> or don't copy the given Surface but creates a new one based on the
> given surface's renddesc. That's not coded yet.
>
> Targets would use its own cairo_surface_t, the one created when it was
> asked to return a CairoSurface by obtain_surface(). Usually the
> procedure for a obtain_surface implementation would be:
> 1) Create a new cairo_surface_t* using the specific backend:
> cairo_xxxxx_surface_create()
> 2) Create a new CairoSurface instance filled with a reference of the
> created cairo_surface_t
> 3) Return de CairoSurface pointer.
I believe everything you say will still work if obtain_surface
returned a cairo_surface_t*. I think that few enough places in the
code (you mentioned only two) call obtain_surface(), so they can do
the burden of wrapping it in a CairoSurface class. There are many more
targets, which would then all have to repeat code.
Also, requests for a temporary storage surface should call
cairo_surface_create_similar() or
cairo_surface_create_similar_image(), while the original surface needs
to be created with more specialized functions depending on the target.
So a new create_similar() method could be added to CairoSurface to fit
that use case.
That's why I suggest that obtain_surface() return a cairo_surface_t*,
and the caller immediately construct a CairoSurface using it. I feel
that it will make the targets a lot simpler if they don't have to deal
with the CairoSurface API. (They already have to use the base cairo
API)
>
>> 3) I think map_cairo_image and unmap_cairo_image should be private
>> methods that are called automatically on an as-needed basis.
>>
> Possibly. I can move to private and see how they are needed from
> outside. If not possible keep private then move to public.
> I'm in the middle of one commit that would implement some changes that
> you might like.
>
>> One option is:
>> -- The CairoSurface constructor automatically maps the surface
>> -- new method: get_cairo_context() creates a context (cairo_t*) and
>> unmaps the surface
>> -- new method: release_cairo_context() deletes the context and remaps
>> the surface to an image
>>
>
> I think that passing a cairo_t* is not needed. I wrote it initially to
> use a cairo_t but finally believed that cairo_t is only used when
> doing drawing operations over the surface so at the end, it can be
> created on the fly based on the carried surface. Anyway, it is a
> change that doesn't represent a big deal because a cairo_t holds a
> cairo_surface_t so it can be changed in any moment.
>
>> That way, the sequence of calls will look something like this:
>> * Create surface
>> * Map it to image
>> * Software render on some layers
>> * Cairo render on one layer
>> -- get_cairo_context() unmaps the image
>> -- cairo drawing operations on the context
>> (all cairo drawing operations operate on a context, not on a surface!)
>> -- release_cairo_context() signals that drawing is done, surface is
>> remapped to an image
>> * Software render on some more layers
>> * Finish render
>>
>> The other option is to optimize for the case where most layers can
>> render using cairo:
>> -- The CairoSurface constructor doesn't map the image
>> -- Calls to etl::surface API, (e.g. blit_to) map to image before
>> proceeding, and unmap when finished
>>
>
> Yes, that is important to study because the implementation of some
> layers can be straight forward (and very efficient) or a waste of
> efficiency. See layer Rotate. If the layer's context is all made by
> Cairo API drawable layers, it could just pass the transformation
> matrix to the layers and they would perform the drawing operations in
> a rotated vector mode. In other case (if there are any layer that
> break up things - i.e. a blur layer) the Rotate layer must perform the
> rotation using the rotate image cairo api, that would be much slower.
>
> So in the case of optimization, We can think that it might be better
> to carry on with cairo_t* instead of cairo_surface_t* because the
> transformation matrix can be carried (and combined) from one layer to
> other... But in general all the drawings operations are enclosed by a
> cairo_save() and cairo_restore() callings, that will not preserve any
> of the previous cairo_t* modifications.That's the reason why I choose
> cairo_surface_t instead of cairo_t. I hope it makes sense.
Makes sense. One reason I'm suggesting keeping a single cairo_t* is
that creating and then destroying one at each layer may be
inefficient. cairo_restore() might clear the settings, but I have no
idea if it deallocates memory or flushes the rendering the same way
destroying a cairo_t* must. We can keep it as just the surface for
now. As you said, it's not a hard thing to change later on.
As always, I'm excited to see the new developments!
~Nikita
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Synfig-devl mailing list
Synfig-devl@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/synfig-devl
