Opened 12 years ago

Closed 11 years ago

#1067 closed bug (fixed)

Printing doesn't work in ShowImage

Reported by: jackburton Owned by: jackburton
Priority: blocker Milestone: R1
Component: Servers/app_server Version: R1/pre-alpha1
Keywords: Cc: axeld@…, host.haiku@…
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

If you try to print from ShowImage, the application will hang. A spool file will be created, but the print_server doesn't seem to recognize it. Printing (somewhat) works from StyledEdit and Terminal. Component=Showimage for now, but could be a print_server or libbe issue. Feel free to redirect as needed.

Attachments (1)

ShowImage2.diff (5.4 KB) - added by julun 11 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 11 years ago by jackburton

Cc: axeld@… added

I tracked down the problem: ShowImage hangs in BPrintJob::DrawView, which itself calls BPicture::Flatten(). Flatten() downloads the picture from the app_server. In this case the picture, being a bitmap, is bigger than most normal pictures (in my test, the Haiku logo - big version), it's 4.825.808 bytes). Adding a Flush() to the BPortLink link, in an intermediate position in the app_server code (ServerPicture::ExportData(), right after StartMessage(B_OK)) fixes the problem (it flushes the link before it gets too big). So I assume this being a problem in the BPortLink implementation.

comment:2 Changed 11 years ago by jackburton

Component: - Applications/ShowImageServers/app_server

comment:3 Changed 11 years ago by jackburton

That said, I agree it's not a good idea to pass that much data through ports. Maybe we should add some code to portlink which automatically uses areas when the data exceeds a certain size ?

comment:4 Changed 11 years ago by jackburton

A doubt just stroke my mind: Can portlink be used like I do in ServerPicture::ExportData() vs Picture::_Download() ? In ExportData I do multiple Attach() while in _Download() I do one big Read().

Is this correct ?

comment:5 in reply to:  4 ; Changed 11 years ago by jackburton

I added some checks, and can confirm that we reach the maximum limit for the buffer for LinkSender.

comment:6 in reply to:  5 Changed 11 years ago by jackburton

Replying to jackburton:

I added some checks, and can confirm that we reach the maximum limit for the buffer for LinkSender.

What we'd need is something like the original portlink implementation in beos: it didn't send whole messages, but a stream of data: so big chunks of data could be sent splitted through the port, and the client would do reads in a loop until the requested size would be fulfilled.

comment:7 Changed 11 years ago by laplace

Owner: changed from laplace to jackburton
Priority: normalblocker

This is a quite serious bug for printing, I propose to change its milestone to R1/alpha1.

comment:8 in reply to:  7 Changed 11 years ago by jackburton

Replying to laplace:

This is a quite serious bug for printing, I propose to change its milestone to R1/alpha1.

I agree, but I don't think I have the time to work on this, sorry. So someone else will have to fix it. Currently, except for what I wrote in comment 6, I don't have a good idea on how to solve this.

comment:9 Changed 11 years ago by laplace

Cc: host.haiku@… added

CCed Karsten as he brought up this topic on printing mailing list.

In R5 BView seems to support a kind of "disk mode" SetDiskMode(filename, offset) according to the comment in our View.cpp the picture is flattened to the file while the drawing commands are recorded, maybe that could be utilized, if SetDiskMode(...) would be implemented ;)?

On the other hand this might be an optimization only and un-/flattening of large bitmaps must be supported anyway?

comment:10 Changed 11 years ago by julun

Hi,

can we do a partial upload in _Upload(), like splitting the data in a defined size and use link.Attach<void*> and link.Read<void*> as long all data are up in the app_server? Same for download? Would that work? This would effect both _Upload()/ _Download in BPicture and ImportData/ExportData in ServerPicture, right? If that could work i can give it a try next week, I'm off over the weekend.

Regards, Karsten

comment:11 Changed 11 years ago by julun

Forget what i wrote, i had a short look at ServerLink.Attach/ Read. Need to think about it again.

comment:12 Changed 11 years ago by julun

Hi,

I've created an version of LinkSender/Receiver using an area to pass data arround the exceeds the max buffer size. I'm not sure if i leak the area in LinkSender. If someone has the time, please review.

Thanks Karsten

comment:13 Changed 11 years ago by laplace

In line 126 and 131 of your patch you have removed the space after "for" statement, and inside the space after each semicolon is also missing.

Why have you removed "new (std::nothrow)" in line 123, 124. This will leak opList, if an exception is thrown when allocation of ptList fails? Either that or if exceptions are disabled, a null pointer access could happen, if memory allocation did fail.

Maybe you could use class AutoDeleter (or whatever it is called) to delete the buffers automatically that are for example allocated in line 976 or 1014.

Otherwise it looks good as far as I can tell, besides in rare cases the area could be leaked on receiver side, if the receiver dies before it can delete the area.

In order to fix that, the sender side somehow needs to keep track of the opened areas and delete them after the receiver has died. BTW is the sender == app_server? The sender also has to be notified, when the receiver deletes the area, so that the sender does not delete it twice. Could be a problem, if the area_id has be reused for a new area. I don't know how likely that is.

In case of bad performance an area cache might be necessary too?

Does printing from ShowImage work now, BTW?

comment:14 Changed 11 years ago by julun

Hi,

I'm sorry for the confusing patch. i made the diff with an not up to date version of my tree. I will add a new one ASAP, also without those whitespaces for easyer review. With this patch printing works fine from ShowImage.

Karsten

comment:15 Changed 11 years ago by julun

Hi,

here is an updated one, I don't know why the first one showed all those strange removal stuff. Hope this one is easyer to review. Thanks for the comments so far. :)

Karsten

comment:16 Changed 11 years ago by jackburton

Ah, yes! That looks very nice, indeed! Moreover, done this way, it will also solve other problems related to big transfers between app_server and libbe (just check Window.cpp). Good work!

comment:17 Changed 11 years ago by jackburton

One more comment, besides what Michael already wrote: since resize_area() can fail, you should check its return value too. And in case it happens, I guess you could try to delete and re-create the area. Thanks for your work, btw!

comment:18 in reply to:  16 Changed 11 years ago by jackburton

Replying to jackburton:

Ah, yes! That looks very nice, indeed! Moreover, done this way, it will also solve other problems related to big transfers between app_server and libbe (just check Window.cpp).

Sorry, it's in View.cpp (for example, BView::FillShape())

comment:19 Changed 11 years ago by stippi

The new patch looks good to me, only you need to use new (std::nothrow) in the first part of the patch (two places) and check the returned pointer. Return B_NO_MEMORY if it's NULL.

comment:20 Changed 11 years ago by julun

Hi all,

thanks for the comments. I have integrated them so far and reduced a bit of the code also. If you agree on that patch i will commit it later today.

Karsten

Changed 11 years ago by julun

Attachment: ShowImage2.diff added

comment:21 Changed 11 years ago by stippi

You are leaking the first buffer in the error case. Looks fine otherwise, but I kind of looked in a hurry... :-)

comment:22 Changed 11 years ago by julun

Resolution: fixed
Status: newclosed

Hi,

thanks, i fixed it and commited in hrev27214.

Karsten

Note: See TracTickets for help on using tickets.