After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 703368 - backend: sync totem_aspect_frame_pick and totem_aspect_frame_paint
backend: sync totem_aspect_frame_pick and totem_aspect_frame_paint
Status: RESOLVED FIXED
Product: totem
Classification: Core
Component: GStreamer backend
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Maintainer alias for GStreamer component of Totem
Maintainer alias for GStreamer component of Totem
Depends on:
Blocks:
 
 
Reported: 2013-07-01 07:49 UTC by Alban Browaeys
Modified: 2013-10-01 07:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backend: sync totem_aspect_frame_paint and totem_aspect_frame_pick (1.70 KB, patch)
2013-07-01 07:49 UTC, Alban Browaeys
none Details | Review
backend: totem_aspect_frame - Fixes pick of the actor. (1.95 KB, patch)
2013-09-28 09:11 UTC, Alban Browaeys
reviewed Details | Review
v2: backend: totem_aspect_frame - Fixes pick of the actor. (1.44 KB, patch)
2013-09-30 13:10 UTC, Alban Browaeys
accepted-commit_now Details | Review
backend: totem_aspect_frame - Fixes pick of the actor. (1.41 KB, patch)
2013-10-01 07:57 UTC, Bastien Nocera
committed Details | Review

Description Alban Browaeys 2013-07-01 07:49:09 UTC
Created attachment 248117 [details] [review]
backend: sync totem_aspect_frame_paint and  totem_aspect_frame_pick

Fixes:
(totem:24473): Clutter-WARNING **: The required ID of 2 does not refer
to an existing actor; this usually implies that the pick() of an actor
is not correctly implemented or that there is an error in the
glReadPixels() implementation of the GL driver.

This one could get fixed the opposite way, ie syncing paint with pick.
Comment 1 Bastien Nocera 2013-07-01 07:51:56 UTC
Lionel?
Comment 2 Lionel Landwerlin 2013-07-01 09:42:44 UTC
Could you fill in the version affected by this problem? 3.6/3.8/master?
Comment 3 Bastien Nocera 2013-07-01 11:31:55 UTC
3.9.x
Comment 4 Lionel Landwerlin 2013-07-02 18:49:01 UTC
I don't think this patch is fixing anything here.
Could you explain a bit more why you think that would be the case?
Comment 5 Bastien Nocera 2013-07-03 14:51:15 UTC
I still see warnings when double-clicking on the video even with this patch:
(totem:31128): Clutter-WARNING **: The required ID of 16764074 does not refer to an existing actor; this usually implies that the pick() of an actor is not correctly implemented or that there is an error in the glReadPixels() implementation of the GL driver.
Comment 6 Alban Browaeys 2013-07-20 02:07:19 UTC
back then I found that the allocation box, did not matched the rectrangle that _paint uses to clip. Thus then pick fails .
I will renew the analysis process in a week or two as I did not get the issue anymore and you did.
Comment 7 Alban Browaeys 2013-09-28 09:11:54 UTC
Created attachment 255970 [details] [review]
 backend: totem_aspect_frame - Fixes pick of the actor.

Fixes:
(totem:24473): Clutter-WARNING **: The required ID of 2 does not refer
to an existing actor; this usually implies that the pick() of an actor
is not correctly implemented or that there is an error in the
glReadPixels() implementation of the GL driver.

Let parent do the pick "draw" for the container as per
Example 24. Pick implementation of a container of the clutter
reference manual version 1.8, Section "Implementing a new actor".

Also in the pick, only call the container while expanded to sync with
the paint.
Comment 8 Lionel Landwerlin 2013-09-30 08:32:59 UTC
Review of attachment 255970 [details] [review]:

I would have replaced :

  cogl_set_source_color4ub (color->red, color->green,			
                            color->blue, color->alpha);			
  cogl_rectangle (box.x1, box.y1, box.x2, box.y2);

by 
  CLUTTER_ACTOR_CLASS (totem_aspect_frame_parent_class)->pick (actor, color);

and left everything else as it is.

I think I now get the problem. The aspectframe widget is probably not a reactive actor, though, because we do a custom pick we're probably painting something that is not listed as an expected actor. So the chain up probably the only thing that needs to be fixed.
Comment 9 Alban Browaeys 2013-09-30 13:10:04 UTC
Created attachment 256082 [details] [review]
v2: backend: totem_aspect_frame - Fixes pick of the actor.

indeed that does the job. Thanks
Comment 10 Bastien Nocera 2013-09-30 13:13:46 UTC
Review of attachment 256082 [details] [review]:

Looks good otherwise to commit to master, gnome-3-8 and gnome-3-10.

::: src/backend/totem-aspect-frame.c
@@ +321,3 @@
   clutter_actor_get_allocation_box (actor, &box);
 
+  CLUTTER_ACTOR_CLASS (totem_aspect_frame_parent_class)->

One-liner?
Comment 11 Alban Browaeys 2013-10-01 07:39:28 UTC
https://bug703368.bugzilla-attachments.gnome.org/attachment.cgi?id=256082 has :
-  cogl_set_source_color4ub (color->red, color->green,
-                            color->blue, color->alpha);
-  cogl_rectangle (box.x1, box.y1, box.x2, box.y2);
+  CLUTTER_ACTOR_CLASS (totem_aspect_frame_parent_class)->
+    pick (actor, color);

looks fine. What would be the one line ?

Cannot commit , my bad for  postponing the commit access process for too long .
Comment 12 Bastien Nocera 2013-10-01 07:57:19 UTC
Created attachment 256159 [details] [review]
backend: totem_aspect_frame - Fixes pick of the actor.

Fixes:
(totem:24473): Clutter-WARNING **: The required ID of 2 does not refer
to an existing actor; this usually implies that the pick() of an actor
is not correctly implemented or that there is an error in the
glReadPixels() implementation of the GL driver.

Let parent do the pick "draw" for the container as per
Example 24. Pick implementation of a container of the clutter
reference manual version 1.8, Section "Implementing a new actor".