https://bugs.launchpad.net/inkscape/+bug/871563/comments/12
Hey KK,
Your message above in that bug report is a little cryptic because the current code and the patches previous suggested use pixbufs, while you talk about cairo surfaces.
Are you saying that the pixbuf code should be removed and replaced with dealing directly in cairo surfaces?
I'm a little confused because the bug is about pasting objects, and I can only assume that we get a pointer to a block of data when someone pastes things in and we stuff that into an image object of some kind.
Could you explain what you mean?
Martin,
2013/9/13 Martin Owens <doctormo@...400...>:
https://bugs.launchpad.net/inkscape/+bug/871563/comments/12
Hey KK,
Your message above in that bug report is a little cryptic because the current code and the patches previous suggested use pixbufs, while you talk about cairo surfaces.
Are you saying that the pixbuf code should be removed and replaced with dealing directly in cairo surfaces?
It's not necessary. The pixbuf can be modified in place to conform to the Cairo pixel format. After that, a Cairo image surface can be created that shares the pixbuf's underlying data. This technique is used in the interactive renderer (src/display) and could also be used in the PDF renderer (src/extension/internal/cairo-*)
I'm a little confused because the bug is about pasting objects, and I can only assume that we get a pointer to a block of data when someone pastes things in and we stuff that into an image object of some kind.
Could you explain what you mean?
The compressed image data should be passed all the way down to the renderer, which should call cairo_set_mime_data() before using the surface that represents the image. In case of embedded images, this is the base64-decoded value of the data href. For linked images, it's the contents of the file.
The relevant function to change is CairoRenderContext::renderImage() in src/extension/internal/cairo-render-context.cpp. The best way to pass the compressed image data around would be to store it directly on the pixbuf, using g_object_set_data_full() in sp_image_update() in sp-image.cpp. The same technique could be used to store the Cairo surface which is currently created per-pixbuf.
Regards, Krzysztof
Just a note - I'm working on fixing this. As a first step, I am improving the GdkPixbuf / Cairo surface shared pixels technique by using g_object_set_data_full() / cairo_surface_set_user_data(), and should commit it soon. The second step will be to use similar techniques to pass the compressed image data around. I should get this done over the weekend.
Regards, Krzysztof
No problem,
Is r12512 part of the work?
Because src/display/cairo-utils.cpp:318 is an awesome code comment. Full on technical respect! :-D
Martin,
On Fri, 2013-09-13 at 22:40 +0200, Krzysztof Kosiński wrote:
Just a note - I'm working on fixing this. As a first step, I am improving the GdkPixbuf / Cairo surface shared pixels technique by using g_object_set_data_full() / cairo_surface_set_user_data(), and should commit it soon. The second step will be to use similar techniques to pass the compressed image data around. I should get this done over the weekend.
Regards, Krzysztof
2013/9/14 Martin Owens <doctormo@...400...>:
No problem,
Is r12512 part of the work?
Because src/display/cairo-utils.cpp:318 is an awesome code comment. Full on technical respect! :-D
Yes, r12512 contains the improvements I've mentioned earlier. You're welcome :)
[For others: the comment is actually at line 381.]
I have around 75% of the fix done. Right now I'm tying some loose ends around the GdkPixbuf image input extension, so I should have working code tomorrow.
Regards, Krzysztof
The fix is committed as r12516.
I tested by importing several multi-megabyte JPEG photos. The file size indicates that they are imported as JPEG. Similarly, the PDF export is much smaller than previously, because the photos are not re-saved as PNG.
This work is somewhat related to the SAX parser work which remains to be done. If we migrate to a SAX parser, we can avoid storing base64-encoded copies of the data in the XML tree - we can decode them on the fly and store only the binary data, which we could pass directly to cairo_image_surface_set_mime_data(). A similar mechanism could be gradually introduced for other attributes which could benefit from alternate forms of storage, such as CSS and ID references.
Regards, Krzysztof
On Sat, 2013-09-14 at 05:48 +0200, Krzysztof Kosiński wrote:
I tested by importing several multi-megabyte JPEG photos. The file size indicates that they are imported as JPEG. Similarly, the PDF export is much smaller than previously, because the photos are not re-saved as PNG.
This is great Krzysztof, this means we're two bugs closer to killing those ten (now ... still ten) blockers. OK so we've got two more, but they look like easy blockers.
I think I'll go after the text input crashes and maybe the path-inset bug after that. We'll see if two more can be killed this weekend.
*high five*
Martin,
P.S. I assume we would/would-not use the sax parser in libxml2 and would this be for input/output and I speculate render my recent fix moot? :-P
2013/9/14 Martin Owens <doctormo@...400...>:
P.S. I assume we would/would-not use the sax parser in libxml2 and would this be for input/output and I speculate render my recent fix moot? :-P
Yes, we would use the libxml2 SAX parser in a C++ wrapper. It would be used for input only, there is nothing to parse when outputting. With a SAX parser, this regex-based fix would not be necessary.
Regards, Krzysztof
On 2013-09-14 05:48 +0200, Krzysztof Kosiński wrote:
The fix is committed as r12516.
One of the recent commits (12512-12516) related to GdkPixbuf / Cairo surface results in crashes when working with SVG files which contain links to "missing" bitmap images.
In my tests on Ubuntu 12.04 and 13.04 (VM, 64bit), inkscape (trunk PPA r12526) crashes immediately when trying to open a test file with a missing image; on OS X 10.7.5, the behavior is more erratic: files sometimes open fine, and crash on selection, or when applying extensions or on quit. Same was reported for recent Windows builds [*].
If the test file can be opened without crash, the placeholder graphic for the missing image is no longer displayed (the one which says 'Linked Image not found'). The missing image (or its placeholder) renders as fully transparent rectangle, but is nevertheless directly selectable with a single LMB click.
Testing with archived builds on OS X 10.7.5: - not reproduced with r12511, - reproduced with r12517 and later builds.
I tested by importing several multi-megabyte JPEG photos. The file size indicates that they are imported as JPEG. Similarly, the PDF export is much smaller than previously, because the photos are not re-saved as PNG.
This work is somewhat related to the SAX parser work which remains to be done. If we migrate to a SAX parser, we can avoid storing base64-encoded copies of the data in the XML tree - we can decode them on the fly and store only the binary data, which we could pass directly to cairo_image_surface_set_mime_data(). A similar mechanism could be gradually introduced for other attributes which could benefit from alternate forms of storage, such as CSS and ID references.
[*] See comments #11-#13 in https://bugs.launchpad.net/inkscape/+bug/1161003
2013/9/19 su_v <suv-sf@...58...>:
On 2013-09-14 05:48 +0200, Krzysztof Kosiński wrote:
The fix is committed as r12516.
One of the recent commits (12512-12516) related to GdkPixbuf / Cairo surface results in crashes when working with SVG files which contain links to "missing" bitmap images.
In my tests on Ubuntu 12.04 and 13.04 (VM, 64bit), inkscape (trunk PPA r12526) crashes immediately when trying to open a test file with a missing image; on OS X 10.7.5, the behavior is more erratic: files sometimes open fine, and crash on selection, or when applying extensions or on quit. Same was reported for recent Windows builds [*].
If the test file can be opened without crash, the placeholder graphic for the missing image is no longer displayed (the one which says 'Linked Image not found'). The missing image (or its placeholder) renders as fully transparent rectangle, but is nevertheless directly selectable with a single LMB click.
Testing with archived builds on OS X 10.7.5:
- not reproduced with r12511,
- reproduced with r12517 and later builds.
Please re-test with r12531. I am unable to reproduce the problem, so probably the changes in r12531 which were also related to image handling have fixed it.
Regards, Krzysztof
On 2013-09-19 03:33 +0200, Krzysztof Kosiński wrote:
2013/9/19 su_v <suv-sf@...58...>:
On 2013-09-14 05:48 +0200, Krzysztof Kosiński wrote:
The fix is committed as r12516.
One of the recent commits (12512-12516) related to GdkPixbuf / Cairo surface results in crashes when working with SVG files which contain links to "missing" bitmap images.
In my tests on Ubuntu 12.04 and 13.04 (VM, 64bit), inkscape (trunk PPA r12526) crashes immediately when trying to open a test file with a missing image; on OS X 10.7.5, the behavior is more erratic: files sometimes open fine, and crash on selection, or when applying extensions or on quit. Same was reported for recent Windows builds [*].
If the test file can be opened without crash, the placeholder graphic for the missing image is no longer displayed (the one which says 'Linked Image not found'). The missing image (or its placeholder) renders as fully transparent rectangle, but is nevertheless directly selectable with a single LMB click.
Testing with archived builds on OS X 10.7.5:
- not reproduced with r12511,
- reproduced with r12517 and later builds.
Please re-test with r12531. I am unable to reproduce the problem, so probably the changes in r12531 which were also related to image handling have fixed it.
Fix in r12531 confirmed, tested on - Ubuntu 12.10 (VM 64bit) (local build) - OS X 107.5
On Ubuntu, the same test files which crash on load with r12530 now open fine (r12531). The placeholder graphic is displayed for missing images again. Same on OS X (comparing r12530 with r12531).
Thanks a lot!
participants (3)
-
Krzysztof Kosiński -
Martin Owens -
su_v