Opened 2 years ago

Closed 23 months ago

Last modified 15 months ago

#4104 closed defect (fixed)

Screenshot is all black in XO-4

Reported by: godiard Owned by: erikos
Priority: High Milestone:
Component: Sugar Version: Unspecified
Severity: Major Keywords: regression, r+, olpc-test-passed
Cc: humitos, erikos, dsd, godiard Distribution/OS: OLPC
Bug Status: Assigned

Description

(And is not a rugby player)

TestCase:
Press Alt-1, go to the Journal and look at the screenshot created, you will see a black rectangle.

Tested in xo-4 and xo-1.75, the error only happens in xo-4.
Nothing in the log.

Other instances generate images like the preview associated to a activity, or the bookmarks in Browse or Read.

Is not solved installing pygobject3-3.3.91 (see #4102)

Attachments (4)

0001-Screenshot-global-key-Alt-1-fixed-SL-4104.patch (2.2 KB) - added by humitos 23 months ago.
0001-Screenshot-global-key-Alt-1-fixed-SL-4104.2.patch (1.5 KB) - added by humitos 23 months ago.
v2 - variable name changed as gonzalo suggested me
0001-Screenshot-global-key-Alt-1-fixed-SL-4104.3.patch (1.4 KB) - added by godiard 23 months ago.
New patch with a better msg
0001-Screenshot-extension-make-sure-we-use-a-compatible-c.patch (1.9 KB) - added by manuq 23 months ago.
Updated commit message.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 2 years ago by humitos

It does work on XO-1.5 too 13.1.0 os8

comment:2 Changed 2 years ago by tonyforster

see also Laptop12277

comment:3 Changed 2 years ago by humitos

  • Cc humitos added

I've just tested this on XO-4 build 13.1.0 os11 and it worked (no more black image)

After a while I took another screenshot and it was the same. I took another and another and they were the same than the first one.

comment:4 Changed 2 years ago by manuq

I can confirm humitos comment, also with xo-4 build os11.

  • take a screenshot, then go to the Journal, open it: all normal.
  • navigate to another place in the Sugar UI, take another screenshot: is the same as the first one

All successive screenshots are the same.as the first one taken.

comment:5 Changed 2 years ago by manuq

  • Keywords regression added

comment:6 Changed 2 years ago by manuq

  • Distribution/OS changed from Unspecified to OLPC

Seems specific to XO-4, so reporting in olpc trac: http://dev.laptop.org/ticket/12314

comment:7 Changed 2 years ago by erikos

  • Bug Status changed from Unconfirmed to Assigned
  • Priority changed from Unspecified by Maintainer to High
  • Severity changed from Unspecified to Major

comment:8 Changed 23 months ago by humitos

  • Cc erikos dsd godiard added
  • Keywords r? added

I attached a patch that works property on XO-4, XO-1.75 and XO-1.5.

I'm not sure what is the exactly difference with the old code. Please, take a look at the commit message and if you know what it is, provide the explanation here. Thanks!

comment:9 Changed 23 months ago by godiard

I think the name of the varibles you are using is not clear.

window_surface = Gdk.Window.create_similar_surface(

window, cairo.CONTENT_COLOR, width, height)

should be:

screenshot_surface = Gdk.Window.create_similar_surface(

window, cairo.CONTENT_COLOR, width, height)

because that surface is not the real window surface, but the destination of the paint operation.

Other than that, I think is ok.

Changed 23 months ago by humitos

v2 - variable name changed as gonzalo suggested me

comment:10 follow-ups: Changed 23 months ago by humitos

The "issue" that I see with the old code is that we are passing a GdkWindow (link) to Gdk.cairo_create (link) instead of a Cairo Context.

comment:11 Changed 23 months ago by godiard

  • Keywords r+ added; r? removed

Reviewed and Tested

comment:12 in reply to: ↑ 10 Changed 23 months ago by erikos

Replying to humitos:

The "issue" that I see with the old code is that we are passing a GdkWindow (link) to Gdk.cairo_create (link) instead of a Cairo Context.

Please update the patch message accordingly.

Changed 23 months ago by godiard

New patch with a better msg

comment:13 Changed 23 months ago by godiard

erikos, new patch with a better header attached

comment:14 in reply to: ↑ 10 Changed 23 months ago by manuq

Replying to humitos:

The "issue" that I see with the old code is that we are passing a GdkWindow (link) to Gdk.cairo_create (link) instead of a Cairo Context.

No, in the current code we use gdk.cairo_create which receives a GdkWindow.

http://developer.gnome.org/gdk3/stable/gdk3-Cairo-Interaction.html#gdk-cairo-create

Changed 23 months ago by manuq

Updated commit message.

comment:15 Changed 23 months ago by manuq

I've found that the issue is the creation of the screenshot surface. Using gdk_window_create_similar_surface works because according to the documentation it creates a more compatible surface. And is the same we are using in the toolkit.

Current approach:

  1. create a cairo context for the root window (gdk_cairo_create) to get its target surface 'window_surface' (cairo_get_target),
  1. create a new surface with the same size as the window 'screenshot_surface' and format cairo.FORMAT_ARGB32 (cairo.ImageSurface)
  1. create a new cairo context with that new surface as target (cairo.Context)
  1. set 'window_surface' as the source of the new cairo context
  1. paint it (cairo_paint) and save it to a PNG file (cairo.Surface.write_to_png)

Browse approach:

  1. To create the new surface 'screenshot_surface' for step 2. above, use gdk_window_create_similar_surface instead. In toolkit (Activity.get_preview) we do the same. I think is better because according to the documentation it creates a more compatible surface.
  1. To set the window surface as the source surface on the new context, step 4. above, use gdk_cairo_set_source_window instead. In toolkit (Activity.get_preview) we use widget.draw(cr) directly which Ithink is a better approach when you have a widget, but we have a Gdk window instead.

comment:16 Changed 23 months ago by manuq

  • Keywords olpc-test-pending added
  • Resolution set to fixed
  • Status changed from new to closed

Pushed 34fb1f8d .

comment:17 Changed 22 months ago by greenfeld

  • Keywords olpc-test-passed added; olpc-test-pending removed

Screenshots work on all four current XO platforms in Sugar 0.98.2/OLPC 13.1.0 os20.

comment:18 Changed 15 months ago by dnarvaez

  • Milestone 0.98 deleted

Milestone 0.98 deleted

Note: See TracTickets for help on using tickets.