Just a few notes of things that I'm seeing when reading the code:
1) generic_pen_row_iterator stores data_ and pitch_as the two only member
variables. Both are public.
2) generic_pen defines as public the begin_x(), end_x(), begin_y and
end_y() members that returns iterators (pointers) to the data_. But data_
pointer in surface is considered private. Doesn't those methods expose the
private data_ pointer outside the class without any prevention? As you
mentioned before, data_ pointer should be accessed by explicit get_data()
member and not that way.
3) generic_pen defines pitch_ as the difference between row starts. We need
to add a set_pitch member to allow store different pitches than the
sizeof(value_type)*w. This way allowing set_pitch() the cairo stride
concept can be replaced by the pitch at the generic_pen. Same happen to
etl::surface, it needs the set_pitch member.
4) There is a flip_v function that flips vertically the surface. This might
be dangerous if the pitch of this surface is used to create other surface
or to assign one surface to other. See line 198 at _surface.h. It is doing
a memcpy of possibly a negative pitch. Is that legal?
5) Same happen when a surface is created based on a size_type (which is a
typedef of a difference_type) or from two pen positions. It uses directly
the s.x and s.y values to create a new data_ memory allocation without
consider that the difference type could store negative values. This is not
in the safe way.
2012/5/28 Carlos López González <genet...@gmail.com>
> Hi!
> I've changed my email address for the synfig-devl mailing list. This is
> the new one.
>
> Just to make sure we're on the same page, I'll add that it's important
>> that every single method in etl::surface use get_data()/set_data()
>> instead of accessing the data pointer directly.
>>
>>
> Sure. It will be that way.
>
>
>> > 2) Change etl::pen to store a pointer to the surface instead of to the
>> > data, and use get_data()/set_data() instead of accessing the data
>> > directly.
>> >
>> > Passing the surface pointer to the pen (at creation time) instead of
>> > directly the data pointer, should be accompanied with the access of the
>> > pitch from the surface api instead of directly the calculation based on
>> > sizeof(value_type)*w. This way the pitch will turn into the cairo stride
>> > when the passed surface is a cairo one and into the classic
>> size_of(Color)*w when the surface is a synfig one.
>> >
>> > I think also that it has a little more of complication. etl::surface
>> has a
>> > constructor based on two pen positions. It expects that the pen has its
>> data pointer pointing to the pen position. See the move_to, move, inc_x,
>> dec_x, etc. pen member functions. So when the pen bases its data pointer on
>> the one provided by surface that should be taken account too.
>>
>> You're right, there's some more complexity here. But nothing that
>> couldn't be handled by adding a member variable to the pen (or maybe
>> an extra argument/overloading set_data())
>>
>> > 3) Create a CairoSurface that inherits from synfig::Surface. Have it
>> > store data as a union of cairo::surface data and etl::surface data,
>> > and convert from one to the other whenever the data type requested
>> > isn't the one it has. Implement get_data()/set_data().
>> >
>> > Inheritance isn't a coding problem to me. I only see one problem with
>> the data lengths. Say that in synfig::Surface we have reserved memory for
>> an amount of N bytes form the size_of(Color) and w and h values. Say that
>> it is M the number of bytes needed from CairoSurface to store an image of
>> wxh. Now let's look the constructors procedures step by step.
>> > In general when a derived class is called the ancestor constructor is
>> called first and then the derived one. If the synfig::Surface is called, it
>> > shouldn't allocate any amount of memory for the data (like it does now
>> in some constructors). If it would do that maybe it happens that the
>> needed memory amount from CairoSurface (M) were bigger than the amount
>> from synfig::Surface (N) and so the allocated memory from synfig::Surface
>> would become unusable because a new malloc is needed.
>> >
>> > If that is right, the memory allocation has to be done in a second call
>> to a > synfig::Surface member and so at the corresponding CairoSurface too.
>> It > might be called bool synfig::Surface::create(), returning true if
>> everything is right.
>>
>> I would just allocate two blocks of memory, one for the synfig data
>> and one for the cairo data. I see this as just a step on the way to a
>> cairo implementation, so a little inefficiency wouldn't hurt. The
>> double-data storage should eventually be fixed (phase 7). I'm thinking
>> of code that looks like:
>>
>> The doubts on steps two and three comes from the suggestion of have a
> union (in the C++ language sense) of the data allocated in memory for the
> surface data (cairo and synfig). If the data is allocated separately there
> is not such mentioned problem.
>
>
>> class CairoSurface : public Surface {
>> enum_type up_to_date;
>> cairo::surface s;
>> value_type* get_data() {
>> if (up_to_date == CAIRO_DATA) {
>> Color-convert the cairo surface, and store it using
>> Surface::set_data()
>> up_to_date = SYNFIG_DATA;
>> }
>> return Surface::get_data(); //get data from parent
>> }
>> cairo::surface* get_cairo_surface() {
>> if (up_to_date == SYNFIG_DATA) {
>> Convert from Surface::get_data() to the cairo surface
>> up_to_date = CAIRO_DATA;
>> }
>> return s;
>> }
>> }
>>
>> I'm confused here. Why do we need to convert one surface to other? if we
> are using a render method we are using that render method all the time.
> Didn't we agree that convert from one kind of surface type to other in the
> middle of the render process is a waste of time? From my point of view,
> once the surface is created (synfig or cairo) it would remain as it is
> until the final shipment to the target device.
>
> What I understand form your approaching is that we would have either one
> surface data allocation type (synfig) or other one surface allocation type
> (cairo) and not both. In both cases the pointer should be the most common
> smaller one (char pointer) and then the synfig::surface would do a pointer
> casting to use the proper one (Color *) instead of the generic (char *)
> that is used directly by Cairo.
>
> So is it needed to force to use get_data() every where to access the char
> data pointer? I think that each class (ancestor or derived should use the
> same pointer and cast it properly when needed. Moving along the data should
> be done with the virtualized members and would change the pointer address
> according to the class that is being used in each moment.
>
> I'm going to take a second think to this and will reply on the mailing
> list later.
>
>
>> > 4) When initializing a Surface in the render target, create a
>> > CairoSurface instead.
>> >
>> > Right
>> >
>> >
>> > 5) Create CairoPen/CairoAlphaPen that use the cairo data from the
>> > surface, where possible.
>> >
>> > Those CairoPen and CairoAlphaPen, would be based on generic_pen and
>> > alpha_pen templates? If so, how are you going to use the templates, with
>> > synfig::Color as base class?
>>
>> They should be based on whatever calling synfig::Surface->get_pen()
>> returns, which I think is synfig's version of the pen (not etl's).
>> That way existing rendering code will not need to be tweaked.
>>
>> OK
>
>
>> In that case synfig::Color would have to be used unchanged. I think
>> that's fine. Colors in the parameter tree are floating-point anyway,
>> and the pen/surface code is a natural place to convert them to the
>> cairo representation. Since cairo deals with bytes, its colors aren't
>> a class so much as a pointer to some data.
>>
>> Right
>
>
>> > 6) Convert the remaining methods in CairoSurface/CairoPen to use
>> Cairo.
>> >
>> > Does it means to use high level Cairo API when possible at CairoSurface
>> and
>> > CairoPen? (for example in replacement of blit_to)
>>
>> I would say implement blit_to using the cairo API. If the high-level
>> API can't do the job, we can always use the low-level API:
>>
>> {
>> cairo_surface_flush(s);
>> unsigned char *data = cairo_image_surface_get_data(s);
>> tweak individual pixels in data
>> cairo_surface_mark_dirty(s); //or
>> cairo_surface_mark_dirty_rectangle(...)
>> }
>>
>> Roger that
>
>
>> >
>> > 7) Remove etl::surface data storage from CairoSurface, and undo
>> > changes to etl::surface and etl::pen
>> >
>> > I don't get this phrase. Can you elaborate it a bit more?
>>
>> This is the cleanup phase. The double-memory representation from part
>> 3 should be changed to use the cairo buffer alone and not store both
>> representations at the same time.
>>
>> In part 1, a virtual get_data() method is added to etl::surface.
>> CairoSurface is required to implement it even though it doesn't use
>> that data format, which should not be the case. In fact, nothing
>> should be calling this method except for the original (non-cairo) pen
>> classes. So it makes sense to remove them (either entirely, or by
>> making them private and thus non-inheritable).
>>
>> It wouldn't be needed if what I said before is confirmed.
>
> --
> 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
>
>
--
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
