I updated my cairo png target so that it now works with the updates to
the target system (see my branch at nikitakit_new_cairo_core_2).
To render, run `synfig -t cairo_png file.sif`
The colors are either shifted or inverted, I haven't figured out the cause yet.
There's also two bugfixes in there.
One is remembering to set cs_image_ to NULL as-needed -- it's
important to always initialize pointers. The other case was that
unmap() destroyed cs_image_ but didn't set it to NULL, and then the
destructor re-destroyed cs_image. The result was that the surface's
reference count reached 0 before the rendering was finished.
The other was to change the signature of
obtain_surface(cairo_surface_t*) to obtain_surface(cairo_surface_t*&).
Since the argument is used to output a pointer to a surface, it needs
to be a reference to a pointer to a surface.
~Nikita
On Sun, Jul 8, 2012 at 4:08 PM, Nikita Kitaev <nikita...@gmail.com> wrote:
> Please pull my latest changes from nikitakit_new_cairo_core_2. The
> first two commits are essential for the code to compile on my machine
> (Ubuntu 12.04). The third changes the cairo API calls used in
> Context::accelerated_cairorender. (Calling cairo_paint() is simpler
> than drawing a rectangle the size of the surface)
>
> ~Nikita
>
> On Wed, Jul 4, 2012 at 11:17 PM, Carlos López González
> <genet...@gmail.com> wrote:
>> Hi!
>> After reading several times your email and make some code reviews I
>> just can agree that you have reason in most of the issues.
>> What was really making me think that way is the influence of the
>> current render system. One of the things that moved me to use
>> start_frame, end_frame and encapsulate everything inside the
>> CairoSurface was the influence of the current Traget_Scanline::render
>> structure, the way that Layer::accelerated_render renders the Surface
>> and how the synfig::render does its job.
>>
>> 1) We don't need to carry on with CairoSurface but with
>> cairo_surface_t or cairo_t between accelerated_render calls. If a
>> CairoSurface is needed at any time just create it on the fly and
>> extract the image part for direct use. Then once finished the job,
>> destroy the CairoSurface (and the cairo_surface_t that it holds too
>> because it is a reference)
>>
>> 2) When a layer doesn't define accelerated_render then the get_color
>> of the context is used. This is coded at the synfig::render function.
>> On contrary to the synfig::render function that received a
>> target2surface target, the cairo version would receive a
>> cairo_surface_t and it would create a CairoSurface on the fly,
>> extracts its image part and write on it the supersampled color
>> obtained from the context. There is not need to write a
>> target2cairosurface class at all.
>>
>> 3) Effectively, the start_frame can be replaced by the obtain_surface
>> member of the target. I'm thinking on modify it to be like this:
>> bool Target_Cairo::obtain_surface(cairo_surface_t*)=0;
>> I think that pass the cairo_surface_t pointer is better than obtain
>> the pointer directly. Remember that cairo_surface_t is never null even
>> if an error occurs so it is better to check out the sanity of the
>> returned cairo_surface_t on a conditional return boolean.
>> Also, the cairo_surface_t will be passed to the accelerated_render
>> member, so it has to be stored somehow in a local variable.
>> In the case of the end_frame it can be formulated like this:
>> bool Target_Cairo::put_surface(cairo_surface_t*)=0;
>> In this case the put_surface would take care of transfer the modified
>> cairo_surface_t to the device. I doubt if it should destroy the
>> cairo_surface_t or leave it to the function that holds the pointer
>> variable.
>>
>> 4) Temporarily surfaces used in the accelerated_render member. When a
>> surface is needed to make temporary renders we can just create a
>> CairoSurface with a w,h size and it will self create a cairo_surface_t
>> image type. It needs a new constructor.
>>
>> I'll start to make those changes this afternoon.
>> Cheers!
>>
>> 2012/7/4 Nikita Kitaev <nikita...@gmail.com>:
>>> 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
>>
>>
>>
>> --
>> Carlos
>> http://synfig.org
>>
>> ------------------------------------------------------------------------------
>> 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
------------------------------------------------------------------------------
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
