Re: [E-devel] [EGIT] [core/efl] master 03/03: evas: Fix support for image_data_get on snapshot

[The Rasterman]

On Thu, 10 Aug 2017 11:57:10 +0900 Jean-Philippe André  said:

yay!

> OK I pushed a patch for this.
> I believe not checking for NULL and still changing the internal state of
> the image object led to this miserable situation.
> 
> 2017-08-09 22:19 GMT+09:00 Carsten Haitzler :
> 
> > On Tue, 18 Jul 2017 21:57:23 -0700 Jean-Philippe ANDRÉ 
> > said:
> >
> > After much bisecting late into the evening... this patch broke
> > enlightenment
> > and makes it reliably crash when you run virtualbox... in texture
> > uploading a
> > seemingly garbage pointer in the image data...
> >
> > at least for now this is a note to "look into this" ... filing
> >
> > > jpeg pushed a commit to branch master.
> > >
> > > http://git.enlightenment.org/core/efl.git/commit/?id=
> > 45c8e5e983d24b23b244513087799edcda862411
> > >
> > > commit 45c8e5e983d24b23b244513087799edcda862411
> > > Author: Jean-Philippe Andre 
> > > Date:   Wed Jul 19 11:43:35 2017 +0900
> > >
> > > evas: Fix support for image_data_get on snapshot
> > >
> > > evas_object_image_data_get() is a legacy API that I made work
> > > with snapshot objects (evas_object_image_snapshot_set()). Some
> > > changes in the engine broke the behaviour and this patch fixes
> > > it.
> > >
> > > When getting the pixels from an FBO, in read-only mode, we need
> > > to create a temporary image (pixels surface) that contains the
> > > copy of the pixels we get from glReadPixels. This image needs
> > > to be deleted afterwards. It is thus stored by the image object
> > > and freed upon _image_data_set() (good) or object deletion (bad).
> > >
> > > FBO + read-write is not supported by this API (it is supported
> > > through buffer_map as the filters had to use that).
> > >
> > > Fixes T5754
> > > ---
> > >  src/lib/evas/canvas/evas_image_legacy.c  | 84
> > ++
> > > +- src/lib/evas/canvas/evas_image_private.h |  1 +
> > >  src/lib/evas/canvas/evas_object_image.c  |  7 +++
> > >  3 files changed, 79 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/lib/evas/canvas/evas_image_legacy.c
> > > b/src/lib/evas/canvas/evas_image_legacy.c index a8593db29b..3f69dca4e6
> > 100644
> > > --- a/src/lib/evas/canvas/evas_image_legacy.c
> > > +++ b/src/lib/evas/canvas/evas_image_legacy.c
> > > @@ -14,6 +14,14 @@
> > > EVAS_IMAGE_API(_o, __VA_ARGS__); \
> > > } while (0)
> > >
> > > +typedef struct _Evas_Image_Legacy_Pixels_Entry
> > > Evas_Image_Legacy_Pixels_Entry; +
> > > +struct _Evas_Image_Legacy_Pixels_Entry
> > > +{
> > > +   Eo*object;
> > > +   void  *image;
> > > +};
> > > +
> > >  EAPI Evas_Object *
> > >  evas_object_image_add(Evas *eo_e)
> > >  {
> > > @@ -552,7 +560,7 @@ evas_object_image_data_set(Eo *eo_obj, void *data)
> > >
> > > Evas_Object_Protected_Data *obj = efl_data_scope_get(eo_obj,
> > > EFL_CANVAS_OBJECT_CLASS); Evas_Image_Data *o = efl_data_scope_get(eo_obj,
> > > EFL_CANVAS_IMAGE_INTERNAL_CLASS);
> > > -   void *p_data;
> > > +   void *p_data, *pixels;
> > > Eina_Bool resize_call = EINA_FALSE;
> > >
> > >
> > > @@ -563,6 +571,13 @@ evas_object_image_data_set(Eo *eo_obj, void *data)
> > > p_data = o->engine_data;
> > > if (data)
> > >   {
> > > +// r/o FBO data_get: only free the image, don't update pixels
> > > +if ((pixels = eina_hash_find(o->pixels->images_to_free, data))
> > !=
> > > NULL)
> > > +  {
> > > + eina_hash_del(o->pixels->images_to_free, data, pixels);
> > > + return;
> > > +  }
> > > +
> > >  if (o->engine_data)
> > >{
> > >   o->engine_data = ENFN->image_data_put(ENDT, o->engine_data,
> > > data); @@ -640,14 +655,28 @@ evas_object_image_data_set(Eo *eo_obj, void
> > > *data) if (resize_call) evas_object_inform_call_image_resize(eo_obj);
> > >  }
> > >
> > > +static void
> > > +_image_to_free_del_cb(void *data)
> > > +{
> > > +   Evas_Image_Legacy_Pixels_Entry *px_entry = data;
> > > +   Evas_Object_Protected_Data *obj;
> > > +
> > > +   obj = efl_data_scope_safe_get(px_entry->object,
> > EFL_CANVAS_OBJECT_CLASS);
> > > +   EINA_SAFETY_ON_NULL_RETURN(obj);
> > > +   ENFN->image_free(ENDT, px_entry->image);
> > > +   free(px_entry);
> > > +}
> > > +
> > >  EAPI void*
> > >  evas_object_image_data_get(const Eo *eo_obj, Eina_Bool for_writing)
> > >  {
> > > EVAS_IMAGE_API(eo_obj, NULL);
> > >
> > > Evas_Image_Data *o = efl_data_scope_get(eo_obj,
> > > EFL_CANVAS_IMAGE_INTERNAL_CLASS);
> > > +   Evas_Image_Legacy_Pixels_Entry *px_entry = NULL;
> > > +   Eina_Bool tofree = 0;
> > > +   void *pixels = NULL;
> > > int stride = 0;
> > > -   void *pixels;
> > > DATA32 *data;
> > >
> > > if (!o->engine_data) return NULL;
> > > @@ -662,25 +691,48 @@ evas_object_image_data_get(const Eo *eo_obj,
> > Eina_Bool
> > > for_writing) ENFN->image_scale_hint_set(ENDT, 

Re: [E-devel] [EGIT] [core/efl] master 03/03: evas: Fix support for image_data_get on snapshot

[Jean-Philippe André]

OK I pushed a patch for this.
I believe not checking for NULL and still changing the internal state of
the image object led to this miserable situation.

2017-08-09 22:19 GMT+09:00 Carsten Haitzler :

> On Tue, 18 Jul 2017 21:57:23 -0700 Jean-Philippe ANDRÉ 
> said:
>
> After much bisecting late into the evening... this patch broke
> enlightenment
> and makes it reliably crash when you run virtualbox... in texture
> uploading a
> seemingly garbage pointer in the image data...
>
> at least for now this is a note to "look into this" ... filing
>
> > jpeg pushed a commit to branch master.
> >
> > http://git.enlightenment.org/core/efl.git/commit/?id=
> 45c8e5e983d24b23b244513087799edcda862411
> >
> > commit 45c8e5e983d24b23b244513087799edcda862411
> > Author: Jean-Philippe Andre 
> > Date:   Wed Jul 19 11:43:35 2017 +0900
> >
> > evas: Fix support for image_data_get on snapshot
> >
> > evas_object_image_data_get() is a legacy API that I made work
> > with snapshot objects (evas_object_image_snapshot_set()). Some
> > changes in the engine broke the behaviour and this patch fixes
> > it.
> >
> > When getting the pixels from an FBO, in read-only mode, we need
> > to create a temporary image (pixels surface) that contains the
> > copy of the pixels we get from glReadPixels. This image needs
> > to be deleted afterwards. It is thus stored by the image object
> > and freed upon _image_data_set() (good) or object deletion (bad).
> >
> > FBO + read-write is not supported by this API (it is supported
> > through buffer_map as the filters had to use that).
> >
> > Fixes T5754
> > ---
> >  src/lib/evas/canvas/evas_image_legacy.c  | 84
> ++
> > +- src/lib/evas/canvas/evas_image_private.h |  1 +
> >  src/lib/evas/canvas/evas_object_image.c  |  7 +++
> >  3 files changed, 79 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/lib/evas/canvas/evas_image_legacy.c
> > b/src/lib/evas/canvas/evas_image_legacy.c index a8593db29b..3f69dca4e6
> 100644
> > --- a/src/lib/evas/canvas/evas_image_legacy.c
> > +++ b/src/lib/evas/canvas/evas_image_legacy.c
> > @@ -14,6 +14,14 @@
> > EVAS_IMAGE_API(_o, __VA_ARGS__); \
> > } while (0)
> >
> > +typedef struct _Evas_Image_Legacy_Pixels_Entry
> > Evas_Image_Legacy_Pixels_Entry; +
> > +struct _Evas_Image_Legacy_Pixels_Entry
> > +{
> > +   Eo*object;
> > +   void  *image;
> > +};
> > +
> >  EAPI Evas_Object *
> >  evas_object_image_add(Evas *eo_e)
> >  {
> > @@ -552,7 +560,7 @@ evas_object_image_data_set(Eo *eo_obj, void *data)
> >
> > Evas_Object_Protected_Data *obj = efl_data_scope_get(eo_obj,
> > EFL_CANVAS_OBJECT_CLASS); Evas_Image_Data *o = efl_data_scope_get(eo_obj,
> > EFL_CANVAS_IMAGE_INTERNAL_CLASS);
> > -   void *p_data;
> > +   void *p_data, *pixels;
> > Eina_Bool resize_call = EINA_FALSE;
> >
> >
> > @@ -563,6 +571,13 @@ evas_object_image_data_set(Eo *eo_obj, void *data)
> > p_data = o->engine_data;
> > if (data)
> >   {
> > +// r/o FBO data_get: only free the image, don't update pixels
> > +if ((pixels = eina_hash_find(o->pixels->images_to_free, data))
> !=
> > NULL)
> > +  {
> > + eina_hash_del(o->pixels->images_to_free, data, pixels);
> > + return;
> > +  }
> > +
> >  if (o->engine_data)
> >{
> >   o->engine_data = ENFN->image_data_put(ENDT, o->engine_data,
> > data); @@ -640,14 +655,28 @@ evas_object_image_data_set(Eo *eo_obj, void
> > *data) if (resize_call) evas_object_inform_call_image_resize(eo_obj);
> >  }
> >
> > +static void
> > +_image_to_free_del_cb(void *data)
> > +{
> > +   Evas_Image_Legacy_Pixels_Entry *px_entry = data;
> > +   Evas_Object_Protected_Data *obj;
> > +
> > +   obj = efl_data_scope_safe_get(px_entry->object,
> EFL_CANVAS_OBJECT_CLASS);
> > +   EINA_SAFETY_ON_NULL_RETURN(obj);
> > +   ENFN->image_free(ENDT, px_entry->image);
> > +   free(px_entry);
> > +}
> > +
> >  EAPI void*
> >  evas_object_image_data_get(const Eo *eo_obj, Eina_Bool for_writing)
> >  {
> > EVAS_IMAGE_API(eo_obj, NULL);
> >
> > Evas_Image_Data *o = efl_data_scope_get(eo_obj,
> > EFL_CANVAS_IMAGE_INTERNAL_CLASS);
> > +   Evas_Image_Legacy_Pixels_Entry *px_entry = NULL;
> > +   Eina_Bool tofree = 0;
> > +   void *pixels = NULL;
> > int stride = 0;
> > -   void *pixels;
> > DATA32 *data;
> >
> > if (!o->engine_data) return NULL;
> > @@ -662,25 +691,48 @@ evas_object_image_data_get(const Eo *eo_obj,
> Eina_Bool
> > for_writing) ENFN->image_scale_hint_set(ENDT, o->engine_data,
> o->scale_hint);
> > if (ENFN->image_content_hint_set)
> >   ENFN->image_content_hint_set(ENDT, o->engine_data,
> o->content_hint);
> > -   pixels = ENFN->image_data_get(ENDT, o->engine_data, for_writing,
> ,
> > >load_error, NULL);
> > +   pixels = ENFN->image_data_get(ENDT, o->engine_data, for_writing,
> ,
> > >load_error, );

Re: [E-devel] [EGIT] [core/efl] master 03/03: evas: Fix support for image_data_get on snapshot

[The Rasterman]

On Tue, 18 Jul 2017 21:57:23 -0700 Jean-Philippe ANDRÉ  said:

After much bisecting late into the evening... this patch broke enlightenment
and makes it reliably crash when you run virtualbox... in texture uploading a
seemingly garbage pointer in the image data...

at least for now this is a note to "look into this" ... filing

> jpeg pushed a commit to branch master.
> 
> http://git.enlightenment.org/core/efl.git/commit/?id=45c8e5e983d24b23b244513087799edcda862411
> 
> commit 45c8e5e983d24b23b244513087799edcda862411
> Author: Jean-Philippe Andre 
> Date:   Wed Jul 19 11:43:35 2017 +0900
> 
> evas: Fix support for image_data_get on snapshot
> 
> evas_object_image_data_get() is a legacy API that I made work
> with snapshot objects (evas_object_image_snapshot_set()). Some
> changes in the engine broke the behaviour and this patch fixes
> it.
> 
> When getting the pixels from an FBO, in read-only mode, we need
> to create a temporary image (pixels surface) that contains the
> copy of the pixels we get from glReadPixels. This image needs
> to be deleted afterwards. It is thus stored by the image object
> and freed upon _image_data_set() (good) or object deletion (bad).
> 
> FBO + read-write is not supported by this API (it is supported
> through buffer_map as the filters had to use that).
> 
> Fixes T5754
> ---
>  src/lib/evas/canvas/evas_image_legacy.c  | 84 ++
> +- src/lib/evas/canvas/evas_image_private.h |  1 +
>  src/lib/evas/canvas/evas_object_image.c  |  7 +++
>  3 files changed, 79 insertions(+), 13 deletions(-)
> 
> diff --git a/src/lib/evas/canvas/evas_image_legacy.c
> b/src/lib/evas/canvas/evas_image_legacy.c index a8593db29b..3f69dca4e6 100644
> --- a/src/lib/evas/canvas/evas_image_legacy.c
> +++ b/src/lib/evas/canvas/evas_image_legacy.c
> @@ -14,6 +14,14 @@
> EVAS_IMAGE_API(_o, __VA_ARGS__); \
> } while (0)
>  
> +typedef struct _Evas_Image_Legacy_Pixels_Entry
> Evas_Image_Legacy_Pixels_Entry; +
> +struct _Evas_Image_Legacy_Pixels_Entry
> +{
> +   Eo*object;
> +   void  *image;
> +};
> +
>  EAPI Evas_Object *
>  evas_object_image_add(Evas *eo_e)
>  {
> @@ -552,7 +560,7 @@ evas_object_image_data_set(Eo *eo_obj, void *data)
>  
> Evas_Object_Protected_Data *obj = efl_data_scope_get(eo_obj,
> EFL_CANVAS_OBJECT_CLASS); Evas_Image_Data *o = efl_data_scope_get(eo_obj,
> EFL_CANVAS_IMAGE_INTERNAL_CLASS);
> -   void *p_data;
> +   void *p_data, *pixels;
> Eina_Bool resize_call = EINA_FALSE;
>  
>  
> @@ -563,6 +571,13 @@ evas_object_image_data_set(Eo *eo_obj, void *data)
> p_data = o->engine_data;
> if (data)
>   {
> +// r/o FBO data_get: only free the image, don't update pixels
> +if ((pixels = eina_hash_find(o->pixels->images_to_free, data)) !=
> NULL)
> +  {
> + eina_hash_del(o->pixels->images_to_free, data, pixels);
> + return;
> +  }
> +
>  if (o->engine_data)
>{
>   o->engine_data = ENFN->image_data_put(ENDT, o->engine_data,
> data); @@ -640,14 +655,28 @@ evas_object_image_data_set(Eo *eo_obj, void
> *data) if (resize_call) evas_object_inform_call_image_resize(eo_obj);
>  }
>  
> +static void
> +_image_to_free_del_cb(void *data)
> +{
> +   Evas_Image_Legacy_Pixels_Entry *px_entry = data;
> +   Evas_Object_Protected_Data *obj;
> +
> +   obj = efl_data_scope_safe_get(px_entry->object, EFL_CANVAS_OBJECT_CLASS);
> +   EINA_SAFETY_ON_NULL_RETURN(obj);
> +   ENFN->image_free(ENDT, px_entry->image);
> +   free(px_entry);
> +}
> +
>  EAPI void*
>  evas_object_image_data_get(const Eo *eo_obj, Eina_Bool for_writing)
>  {
> EVAS_IMAGE_API(eo_obj, NULL);
>  
> Evas_Image_Data *o = efl_data_scope_get(eo_obj,
> EFL_CANVAS_IMAGE_INTERNAL_CLASS);
> +   Evas_Image_Legacy_Pixels_Entry *px_entry = NULL;
> +   Eina_Bool tofree = 0;
> +   void *pixels = NULL;
> int stride = 0;
> -   void *pixels;
> DATA32 *data;
>  
> if (!o->engine_data) return NULL;
> @@ -662,25 +691,48 @@ evas_object_image_data_get(const Eo *eo_obj, Eina_Bool
> for_writing) ENFN->image_scale_hint_set(ENDT, o->engine_data, o->scale_hint);
> if (ENFN->image_content_hint_set)
>   ENFN->image_content_hint_set(ENDT, o->engine_data, o->content_hint);
> -   pixels = ENFN->image_data_get(ENDT, o->engine_data, for_writing, ,
> >load_error, NULL);
> +   pixels = ENFN->image_data_get(ENDT, o->engine_data, for_writing, ,
> >load_error, ); 
> /* if we fail to get engine_data, we have to return NULL */
> if (!pixels) return NULL;
>  
> -   o->engine_data = pixels;
> -   if (ENFN->image_stride_get)
> - ENFN->image_stride_get(ENDT, o->engine_data, );
> -   else
> - stride = o->cur->image.w * 4;
> +   if (!tofree)
> + {
> +o->engine_data = pixels;
> +if (ENFN->image_stride_get)
> +  ENFN->image_stride_get(ENDT, o->engine_data, );
> +