From: Carlos L. G. <gen...@gm...> - 2012-05-28 15:13:30
|
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 |