| | <<<Back 1 day (to 2016/10/28) | 20161029 |
| sebras | hm, fz_new_image_from_pixmap() keeps the pixmap supplied as argument, but doesn't keep the mask. I wonder why... | 08:19.10 |
| | it _does_ however drop the mask in the case of error, suggesting that it propagates the mask reference to the fz_image if a mask is given. | 08:23.14 |
| | fz_new_image_from_compressed_buffer() on the other hand _doesn't_ drop the mask, suggesting that it doesn't intend to propagate any mask reference to fz_image. | 08:23.55 |
| | this seems contradictory. | 08:24.02 |
| | Robin_Watts: you around? | 10:36.34 |
| Robin_Watts | I am. | 10:37.14 |
| sebras | Robin_Watts: so I'm seeing a file that causes a call to fz_fill_image(). | 10:38.48 |
| | now this call has alpha set to 1 | 10:39.07 |
| Robin_Watts | fz_new_image should have a try/catch in to drop the mask in case of error, I think. | 10:39.29 |
| sebras | but as we progress through fz_draw_fill_image() | 10:39.30 |
| | we notice that pixmap->colorspace == rgb | 10:39.44 |
| | and the desired model is == NULL. | 10:39.58 |
| | that doesn't seem healthy, does it..?! | 10:40.06 |
| Robin_Watts | That seems odd. | 10:40.23 |
| sebras | Robin_Watts: why shoudl fz_image drop the mask when it doesn't drop the pixmap? | 10:40.54 |
| | I might have caused this issue myself somehow, but I don't understand how. :) | 10:41.22 |
| Robin_Watts | sebras: fz_new_image does: image->mask = mask; | 10:41.35 |
| | which means it's *taking* the reference to the mask. | 10:41.44 |
| | Either that is right (in which case it should ALWAYS take the reference, which means dropping in the case of an error), or it's wrong (in which case it should be image->mask = fz_keep_blah(mask) | 10:42.30 |
| sebras | Robin_Watts: yeah, I'm leaning towards the altter. | 10:42.43 |
| | the reason being that fz_new_image_from_compressed_buffer() does not handle mask being dropped. | 10:43.08 |
| | in case of error, but fz_new_image_from_pixmap() did. | 10:43.21 |
| | I'm thinking that it is easier to understand if fz_new_image() does fz_keep_image(mask) and whomever calls fz_new_iamge_from_{comrpessed_buffer,pixmap}() have to sort out dropping the reference themselves (turns out this happens in a single place). | 10:44.23 |
| | that way pixmap and mask supplied to fz_new_image_from_pixmap() are being handled in the same way, i.e. here, look at this thing _I_ own, if you want to keep it take a reference yourself. | 10:45.09 |
| Robin_Watts | We want it to be consistent, yes. | 10:47.59 |
| | Either it's consistently "here are some things I own, take references if you want", or it's "I'm passing these to you. They are your responsibility whether you succeed or fail." | 10:48.36 |
| sebras | mm, in this case half of the arguments were of one type and half were of the other. :-/ | 10:49.00 |
| | though I disocvered that fz_compressed_buffer is not fully reference counted (the fz_buffer it contains is though!) | 10:49.35 |
| | so there is a fz_drop_compressed_buffer() but no corresponding keep... | 10:49.49 |
| | Robin_Watts: http://git.ghostscript.com/?p=user/sebras/mupdf.git;a=blobdiff;f=source/fitz/draw-device.c;h=fd74474edc5550efe6cf365f367c2611c4eb6ebc;hp=f587d38e67ef422d7b5a041a4c404891eed010b8;hb=ef2611940d27ee109ec40d20397b6127b38f5a54;hpb=a6e9ef25ae235c177427e2f1602a7e413ec066cd | 10:50.30 |
| | Robin_Watts: in this change I elimited 'after' as I assumed that model would never be NULL. | 10:50.48 |
| | Robin_Watts: why would model be NULL? | 10:50.59 |
| | Robin_Watts: http://pastebin.com/raw/4ds1nQU4 | 10:52.59 |
| Robin_Watts | The purpose of after is to ensure that we don't do extra work. | 10:54.32 |
| sebras | Robin_Watts: I know, I just eliminated the variable, not the check. :) | 10:54.51 |
| Robin_Watts | consider the case where we are plotting to an rgb destination, and we have a greyscale input... | 10:54.57 |
| sebras | it was only used once. | 10:54.58 |
| Robin_Watts | oh, right... | 10:55.02 |
| | Ah, right, so if I'm reading that backtrace right, we are in a softmask definition. | 10:56.15 |
| sebras | Robin_Watts: mmm, but somewhat surprisingly mask_colorspace == rgb! :) | 10:56.38 |
| | in begin_soft_mask(). | 10:57.13 |
| Robin_Watts | There are 2 types of softmask; one of which takes the alpha value of the rendered thing, the other one takes the luminosity (i.e. converts the rendered image to a greyscale and then uses that as alpha) | 10:57.43 |
| | Also, I don't entirely trust that badly formed PDF files (i.e. ones that pop more/less than they should) won't cause strange nestings of colorspaces in the draw device. | 10:58.54 |
| | I think we should cope with such cases, but not be surprised if the rendering doesn't make sense. | 10:59.17 |
| sebras | if they do, shouldn't we detect that early on and bail? | 10:59.17 |
| Robin_Watts | in the drawing functions? probably, yes. | 10:59.34 |
| | ideally we should pop back to sanity and continue with drawing the rest of the page though. | 10:59.53 |
| | OR, in cases where we run out of memory when allocating pixmaps etc. | 11:00.12 |
| | but I assume the file that shows this doesn't show an errors or warnings. | 11:00.32 |
| sebras | Bug691289.pdf page 1, and the xobject being rendered it x6 | 11:01.06 |
| Robin_Watts | Just as a note, for each function that you figure out the "ownership" details of, it would be good if we could make sure the comments document it clearly. | 11:01.25 |
| sebras | no, because I removed the after variable it crashes later on because I don't handle model being == NULL. | 11:01.38 |
| Robin_Watts | (like "This function takes ownership of the references passed in.") | 11:01.39 |
| | sebras: Right. | 11:01.50 |
| sebras | Robin_Watts: yes, I agree. I try to specify this by making the functions be suffixed _drop(). | 11:02.17 |
| Robin_Watts | mask_colorspace = the colorspace of the mask definition. | 11:02.25 |
| | sebras: I'm not sure I agree that all functions should be named _drop. In some cases it's a normal thing to expect. | 11:02.49 |
| sebras | Robin_Watts: right, but I haven't encountered problems where _drop() didn't make sense yet. :) | 11:03.21 |
| | Robin_Watts: and if it _does_ make sense to use _drop() I prefer that to having a comment in a header because of the visibility of the function name. | 11:03.48 |
| Robin_Watts | fz_new_image_drop() would be bad, IMHO. | 11:03.57 |
| sebras | Robin_Watts: absolutely! | 11:04.03 |
| | Robin_Watts: so in this case I'm opting to have fz_new_image() not swallow the mask reference it might be given. | 11:04.38 |
| Robin_Watts | IF that makes the calling code simpler, sure. | 11:08.52 |
| sebras | Robin_Watts: in this case we already had callers that had made mistakes beacuse of this. | 11:09.57 |
| Robin_Watts | I have to take my doggy to the vets for a post-op check. bbs. | 11:11.31 |
| sebras | Robin_Watts: oh! I hope both are doing well. | 11:12.07 |
| Robin_Watts | sebras: Yeah, she had a syst taken off her eyelid earlier in the week. Seems fine after that, but dislikes wearing the cone of shame. | 11:12.48 |
| sebras | Robin_Watts: have you tried this? http://i.imgur.com/vGGnFCo.jpg | 11:15.10 |
| Robin_Watts | haha. | 11:51.32 |
| | tennis balls. | 11:51.35 |
| sebras | Robin_Watts: ok, so I see a mistake I did. | 11:54.55 |
| | Robin_Watts: in my 'after' change I messed up a check. | 11:55.58 |
| | though, it revealed an interesting case that we don't handle. | 11:56.14 |
| | because of this mistake we call fz_convert_pixmap() which attempts to create a pixmap with colorspace == NULL (because the destination is the alpha-only softmask), but also with alpha == 0 (because the original image has no alpha). | 11:59.14 |
| | Robin_Watts: and so fz_new_pixmap() calls fz_new_pixmap_with_data() which asserts. | 11:59.30 |
| | Robin_Watts: but I guess it could be argued that we should be able to handle this case as we could conceivably guess what the alpha value ought to be. | 12:00.31 |
| | Robin_Watts: http://git.ghostscript.com/?p=mupdf.git;a=blob;f=source/fitz/pixmap.c;hb=HEAD#l39 -ve... it took me quite some time to realize that this was not a typo. | 12:25.47 |
| | tor8: Robin_Watts (for the logs): there's a whole slew of commits on sebras/master for different issues I've found and that have been pending. | 18:41.01 |
| | I only noticed that the UCDN we had as old today, it could have been included before 1.10. | 18:42.19 |
| | oh, and we have duplicates of the data, both in harfbuzz and in fitz itself. | 18:42.34 |
| | maybe we ought to include thirdparty/harfbuzz/src/hb-ucdn/unicode_data_db.h instead so we don't have to update both. | 18:48.18 |
| | I have more leaks pending, but they'll have to wait until a later time. | 18:49.45 |
| | Forward 1 day (to 2016/10/30)>>> | |
Log of #ghostscript at irc.freenode.net.
