[cairo] [cairo-commit] src/cairo-skia-surface.cpp

[RAVI NANJUNDAPPA]

Hi Uli, 

> -----Original Message-----
> From: Uli Schlachter [mailto:psychon at znc.in]
> Sent: Monday, July 14, 2014 2:30 PM
> To: nravi.n at samsung.com
> Cc: cairo
> Subject: Re: [cairo] [cairo-commit] src/cairo-skia-surface.cpp
> 
> Hi,
> 
> On 14.07.2014 10:36, RAVI NANJUNDAPPA wrote:
> > In such a case,"return  _cairo_surface_create_in_error (_cairo_error
> > (CAIRO_STATUS_INVALID_FORMAT));"  ( the earlier code) makes more
> sense, as it returns a NULL backend surface with the valid status result.
> > Let me know if I'm missing anything.
> 
> The caller will interpret the NULL as "not supported" and will then "do
> something else" to try and continue. If you return an error surface, then
it
> will just stop trying. Look e.g. at cairo_surface_create_similar() or
> _cairo_surface_create_scratch().
> 

Ok. I understand that now. Thanks for the comments. 

> Also, I just noticed the following:
> 
> From cairo-skia-surface.cpp:
> 
> [...]
> static const struct _cairo_surface_backend cairo_skia_surface_backend = {
>     CAIRO_SURFACE_TYPE_SKIA,
>     _cairo_skia_surface_create_similar,
>     _cairo_skia_surface_finish,
>     _cairo_skia_surface_acquire_source_image,
>     _cairo_skia_surface_release_source_image,
>     _cairo_skia_surface_acquire_dest_image,
>     _cairo_skia_surface_release_dest_image,
> [...]
> 
> From cairo-surface-backend-private.h:
> 
> [...]
> struct _cairo_surface_backend {
>     cairo_surface_type_t type;
>     cairo_warn cairo_status_t
>     (*finish)			(void			*surface);
>     cairo_t *
>     (*create_context)		(void			*surface);
>     cairo_surface_t *
>     (*create_similar)		(void			*surface,
> 				 cairo_content_t	 content,
> 				 int			 width,
> 				 int			 height);
>     cairo_surface_t *
>     (*create_similar_image)	(void			*surface,
> 				 cairo_format_t		format,
> 				 int			 width,
> 				 int			 height);
>     cairo_image_surface_t *
>     (*map_to_image)		(void			*surface,
> 				 const cairo_rectangle_int_t  *extents);
>     cairo_int_status_t
>     (*unmap_image)		(void			*surface,
> 				 cairo_image_surface_t	*image);
> [...]
> 
> Since cairo-skia seems to be unmaintained and no even noticed that it does
> not work AT ALL in ages, can't we just drop it? (Does this even compile? I
> would have expected the C++ compiler to complain loudly about these
> implicit casts.)
> 
> I will from now on ignore cairo-skia.

I just started getting the basic Skia build free of initial hiccups and
there is lot of effort involved in making 
Skia backend more stable, which I feel is a continuous effort.  There is lot
of code cleanup required as well.
I understand the initial state of code is annoying but it'll be always
helpful if we get 3rd eye to catch up such error prone code.
I hope this is fine.

- Thanks 
N Ravi

> 
> Cheers,
> Uli
> 
> > ------- Original Message -------
> > Sender : Uli Schlachter<psychon at znc.in> Date : Jul 14, 2014 13:44
> > (GMT+05:30) Title : Re: [cairo] [cairo-commit]
> > src/cairo-skia-surface.cpp
> >
> > Hi Ravi,
> >
> > On 14.07.2014 08:42, RAVI NANJUNDAPPA wrote:
> >> Hi Chris,
> >>
> >> Please see if this patch fulfils the need.
> >>
> >> Thanks,
> >> N Ravi
> >>
> >>> -----Original Message-----
> >>> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> >>> Sent: Friday, July 11, 2014 11:28 AM
> >>> To: RAVI NANJUNDAPPA
> >>> Cc: cairo at cairographics.org; cairo-commit at cairographics.org
> >>> Subject: Re: Re: [cairo] [cairo-commit] src/cairo-skia-surface.cpp
> >>>
> >>> On Fri, Jul 11, 2014 at 04:33:52AM +0000, RAVI NANJUNDAPPA wrote:
> >>>> Hi Chris,
> >>>>
> >>>> I understand your point.
> >>>> May be in such a case, where the user has not explicitly passed any
> >> format
> >>> type, then we can fallback to   "
> _cairo_image_surface_create_with_content
> >>> (content, width, height);" statement,
> >>>> which ideally should be the case (I'm using this from
> >>> _cairo_gl_surface_create_similar()).
> >>>> I hope this is fine ?
> >>>
> >>> If the create_similar backend returns NULL, it is an indication that
> >>> the operation is unsupported for that backend. The caller then
> >>> decides how to handle the unsupported operation. In the user facing
> >>> function, an image surface is created instead.
> >>> -Chris
> >
> > The attached effectively reverts your change and makes this return
> > NULL again, but it also adds a comment explaining why this shouldn't
make
> a difference.
> >
> > What Chris meant is: The old code returned NULL and that is the right
> > thing to do. The calling function has to be able to handle NULL and
> > fall back in this case. But if you return an error surface, no
> > fallback happens and the error is propagated. And implementing the
> > fallback here in cairo-skia isn't necessary and would be inconsistent
with
> other backends.
> >
> > This isn't a git formatted patch on purpose. Someone else can handle
this.
> >
> > Uli
> >
> 
> 
> --
> "Every once in a while, declare peace. It confuses the hell out of your
> enemies"
>  - 79th Rule of Acquisition