| <<<Back 1 day (to 2016/10/16) | 20161017 |
sebras | tor8: have you had time to look at sebras/master? | 10:46.17 |
| tor8: robin had some comments about the gif patches that I intend to address, but nothing about the other commits. do you? | 10:48.06 |
Robin_Watts | sebras: So TIFFs are restricted to subsampling no more than 4 in any given direction? | 10:56.39 |
| and U and V are always subsampled the same? | 10:57.02 |
| actually, I see from your comment that the latter must be true. | 10:57.57 |
| Can we not have 0x81 say? | 10:58.06 |
| or 0x88 ? | 10:58.46 |
sebras | Robin_Watts: nope, that's true. | 10:58.47 |
| Robin_Watts: at least from reading the spec. | 10:58.53 |
| Robin_Watts: and the rgb2ycbcr tool | 10:59.15 |
| the output from the tool that is. | 10:59.28 |
| Robin_Watts: oh and as I read the tiff spec bitspersample == 8 at all times. | 11:02.36 |
Robin_Watts | sebras: Final commit seems OK to me. I think there are optimisations to be had, but whether it makes sense to put effort into what probably will never make a difference I don't know. | 11:03.21 |
tor8 | sebras: I have no comments. if Robin has LGTM'd the commits I'm happy to have them in before the release. | 11:04.04 |
| Robin_Watts: sebras: one commit on tor/master that probably ought to make it in as well | 11:04.17 |
Robin_Watts | The reload one? | 11:05.41 |
tor8 | yes, the reload one. | 11:05.47 |
sebras | tor8: the reload one looks ok to me. | 11:05.49 |
tor8 | the other one is still in progress | 11:05.54 |
Robin_Watts | I hate the fact we're going all staticy again, but sure. | 11:06.08 |
| If you reload and the document is broken, does it crash? | 11:06.23 |
tor8 | yes, it will bail | 11:06.33 |
sebras | Robin_Watts: yes, there certainly are optimizations to be had. | 11:06.37 |
tor8 | the gl viewer has always been full of globals | 11:06.47 |
Robin_Watts | tor8: Yeah, I know (about the globals). | 11:07.00 |
| The non gl viewer, doesn't crash (or exit), it invents an empty file, so that subsequent reloads (when, say, the file finishes being regenerated) work. | 11:07.43 |
sebras | Robin_Watts: are you thinking about some more elaborate one's than not using -> in for()? | 11:08.00 |
Robin_Watts | so it lgtm, but I predict it will get people whinging about it. | 11:08.02 |
| sebras: possibly. | 11:08.09 |
sebras | Robin_Watts: Re: reload: I think there's a but about make_fake_doc() not working though..? causing a sigsegv. | 11:08.33 |
tor8 | Robin_Watts: how about we add a new document type: fz_new_error_document(ctx, "message")? | 11:08.39 |
Robin_Watts | but as I say, let's wait until it's an issue. | 11:08.50 |
| tor8: Possibly, yes. Would be nice to ensure that the page is kept too. | 11:09.18 |
tor8 | when we get the eventual complaints | 11:09.22 |
Robin_Watts | dives into gs bug. | 11:09.44 |
sebras | tor8: if we do faz_new_error_document(ctx, msg), we should make sure we don't keep any info about the previous document being dirty so we can accidentally save it though. | 11:10.24 |
sebras | is really grateful that the normal length for a fz_try() apperas to be no more than 20 lines of code. | 11:35.06 |
| for some, e.g. the one in pdf_load_hints() it might be a bit difficult to determine what is dropped when and why. | 11:35.50 |
| we seldom nest fz_try(), and when we do, never more than two levels, thankfully. makes my awk script easier. ;) | 11:36.29 |
tor8 | sebras: ah, checking that we don't return in the middle of a try or always block? | 11:39.51 |
sebras | tor8: kind of, I'm fearing that we make more mistakes in the longer fz_try() blocks so I tried to find out where they are. | 11:41.25 |
tor8 | sebras: I'd say flag any nested try blocks too. might as well simplify or rewrite those. | 11:42.17 |
sebras | tor8: hm... would it make sense to do dp->xres = sp->xres; and the same fo yres in fz_convert_pixmap()? we already assume that their sizes are the same, right...? | 11:42.18 |
| tor8: at some places we do assign xres/yres before/after the convert call, in other's (the one's I wrote? :) ) we dont'. | 11:43.10 |
tor8 | sebras: considering how and where fz_convert_pixmap is called, it might make more sense to change its functionality | 11:43.55 |
sebras | tor8: or maybe just a fz_new_pixmap_from_pixmap(fz_context *ctx, fz_colorspace *desired_colorspace)? | 11:43.59 |
tor8 | to fz_new_converted_pixmap(ctx, destination_colorspace, source_pixmap); | 11:44.08 |
sebras | tgmta. | 11:44.30 |
| but your naming is better. | 11:44.40 |
tor8 | we almost always drop the original pixmap, so a wrapper fz_new_converted_pixmap_drop (like the pdf_dict_put_drop functions) could possibly also be helpful | 11:46.11 |
sebras | mmm, after fixing a few I think the logic for freeing is easier and requires less code with such a helper. | 11:47.44 |
| tor8: though we need to know if alpha is desired too. | 11:47.56 |
| how about fz_new_converted_pixmap(ctx, cs, alpha, pix)... or does pix come before cs and alpha? | 11:48.16 |
tor8 | fz_new_converted_pixmap(ctx, pixmap, colorspace, alpha) then? | 11:48.18 |
sebras | ok. | 11:50.10 |
| tor8: oh, pixmaps have x/y?! | 11:52.01 |
tor8 | yes. an offset to keep track of tiles and bands and overlays, etc. | 11:55.31 |
| and glyph pixmap origin offsets | 11:55.44 |
sebras | tor8: oh, then I'll copy x/yrex, x/y and interpolate when doing conversion. assuming that's what one would want. | 11:56.10 |
tor8 | copy everything, pretty much | 11:56.32 |
| copy x,y,w,h,interpolate,xres,yres | 11:57.08 |
| if you end up calling it with ...(ctx, pix, cs, pix->alpha) everywhere, also copy the alpha | 11:57.52 |
sebras | tor8: in fz_draw_fill_image() we seem to allow model to be == NULL and in that case we assume alpha. | 12:04.47 |
| I'm not sure I get that, what does NULL mean in this case? | 12:04.57 |
| oh, alpha only right? | 12:05.37 |
tor8 | alpha only, no color | 12:05.44 |
sebras | tor8: I think I know why we often see "soft mask should be grayscale"... if (tile->n != 1) { ... if (tile->n != 2) fz_warn("soft mask should be...."); .... } | 12:11.59 |
Robin_Watts | sebras: That's probably a hang over from when we always had alpha. | 12:18.23 |
sebras | Robin_Watts: mmm, are fz_image->n supposed to include alpha or not? | 12:19.01 |
Robin_Watts | image->n includes alpha. | 12:19.17 |
| (as it always has). | 12:19.43 |
| It's just that now we can have 0 or 1 alpha planes (as indicated by image->alpha) | 12:20.00 |
sebras | Robin_Watts: fz_new_image() just picks n from the input colorspace, but that is without alpha, right? | 12:20.04 |
Robin_Watts | sebras: Yes. | 12:20.15 |
sebras | Robin_Watts: and if there is no colorspace (i.e. alpha-only) then n == 1. | 12:20.42 |
| Robin_Watts: so alpha is included if it is the _only_ compoment, but not otherwise. | 12:20.59 |
| Robin_Watts: there is no image->alpha. | 12:21.41 |
tor8 | Robin_Watts: so... all the PDF image specific fields in fz_image (bpc, decode, color_key, etc), shouldn't they be put in a subclass like fz_raw_image? | 12:47.43 |
Robin_Watts | tor8: Probably, yes. | 12:57.03 |
| sebras: sorry, I was thinking of pixmap. | 12:57.31 |
| tor8: I've been moving the fz_image stuff towards being more subclass orientated, but haven't got all the way there yet. | 12:58.20 |
sebras | what is fz_alpha_from_gray() meant to do with the luminosity parameter? | 13:28.58 |
Robin_Watts | sebras: hmm. Nothing now, it would seem. | 13:29.49 |
| I think possibly previously it determined whether we worked on alpha or gray? | 13:30.03 |
sebras | it used to be if (!luminosity) sp++; in fz_alpha_from_gray(). | 13:31.12 |
| Robin_Watts: so yes, that seems plausible. | 13:31.19 |
| Robin_Watts: that means we can now remove the luminosity argument as I see it. | 13:31.31 |
Robin_Watts | yes. | 13:35.06 |
tor8 | sebras: if luminosity is true, we should use the gray and discard the alpha; otherwise use the alpha only | 13:54.30 |
| that may be possible to guess based on the properties of the incoming pixmap though | 13:54.52 |
sebras | enough for today. until tomorrow I bid you farwell! | 17:00.55 |
Robin_Watts | night. | 17:03.01 |
| Forward 1 day (to 2016/10/18)>>> | |