| | <<<Back 1 day (to 2013/01/29) | 2013/01/30 |
| Robin_Watts | marcosw: Sorry, was away. | 00:15.38 |
| marcosw | Robin_Watts: that's okay, I forgot what I needed you for. | 00:16.04 |
| tor8 | marcosw: what are you grepping? | 00:16.04 |
| marcosw | tor8: I'm running apt-file search, it's grepping for me | 00:16.41 |
| Robin_Watts | tor8: I think I have the pass by reference stuff working here. | 00:16.50 |
| | We should put in your rect/irect reviews first though. | 00:17.02 |
| | And I think it makes sense to both both of them in together. | 00:17.19 |
| tor8 | Robin_Watts: and also do performance comparisons to see if it's worth upsetting every single user with | 00:17.31 |
| Robin_Watts | tor8: ok. | 00:17.57 |
| tor8 | but yes, since I'm going to break compatibility with the rect/irect stuff it makes sense to push them together | 00:18.08 |
| | but I am curious to see if it makes a noticeable difference | 00:18.21 |
| Robin_Watts | no, I meant put the "rect" and "irect" reviews in together. | 00:18.29 |
| tor8 | oh, right. yes. | 00:18.51 |
| Robin_Watts | I am still worried by the fact that we have fz_pixmap_get_rect that returns a rectangle, that's really a bbox. | 00:19.39 |
| | Or am I still not understanding ? | 00:19.45 |
| | (fz_pixmap_rect, even) | 00:20.45 |
| tor8 | Robin_Watts: I don't understand what you mean. are you complaining about the name or something else? | 00:21.05 |
| Robin_Watts | tor8: I'm complaining that when I'm dealing with something that is innately integer, I'm being given floats. | 00:21.37 |
| | The size/position of pixmaps are all integer things. | 00:21.59 |
| | Getting that as a bbox makes sense. | 00:22.09 |
| | Getting it as a rect... less so. | 00:22.14 |
| tor8 | yes, but none of the other things that are bounding boxes are. and the source of the pixmap bounding boxes come from floating point calculations. | 00:22.30 |
| Robin_Watts | tor8: eh? | 00:22.42 |
| tor8 | the bounding boxes of lots of things are not integers. | 00:22.58 |
| | *only* the pixmaps have integer bounding boxes | 00:23.08 |
| Robin_Watts | Right, so can we get the pixmap bounding boxes as an irect then ? | 00:23.34 |
| tor8 | and the way we arrive at those bounding boxes is by transforming a float bounding box and then rounding it because we want to draw to pixels in it | 00:23.36 |
| | so everywhere *except* the pixmaps, bounding boxes should be float | 00:23.53 |
| Robin_Watts | tor8: Right. So we have rounded and unrounded things. | 00:24.05 |
| tor8 | and a float can perfectly represent any sane integer value in the ranges we care about | 00:24.10 |
| Robin_Watts | rounded things are irects, unrounded are rects. | 00:24.14 |
| tor8 | so yes, the pixmaps should use irects | 00:24.31 |
| | but *nothing* else should | 00:24.35 |
| Robin_Watts | Ah, right, then I think I'm happy. | 00:24.43 |
| tor8 | we used to have an unholy mix of the two, that annoyed me, so I fixed it by making them all float. you're unhappy about pixmap bboxes in the api using floats to represent integers even though it's perfectly safe to do so. | 00:25.32 |
| Robin_Watts | Safe in that fz_rects are capable of representing every value we care about. | 00:26.13 |
| tor8 | correct. now I'm okay with introducing a fz_irect for use with pixmaps and the draw device to make you happier. | 00:26.48 |
| Robin_Watts | That would indeed make me happier. | 00:27.00 |
| | I would feel uneasy about making API entry points take rects, when actually they rely on the rects actually being integer valued. | 00:27.32 |
| tor8 | and passing rects and matrixes by const reference, I have no strong opinion. the code reads easier and is less easy to mess up by accidentally modifying an input value if we pass structs by value, though. | 00:27.46 |
| Robin_Watts | The code does read easier using by value, I will admit that. | 00:28.06 |
| tor8 | Robin_Watts: yes, that is a valid concern. the current patch always rounds the input to such functions before using. | 00:28.19 |
| Robin_Watts | But the cost for that is lots of extra copying. | 00:28.28 |
| | With passing by reference all copies are explicit. | 00:28.37 |
| tor8 | Robin_Watts: you can chain things that you couldn't (like the fz_concat(fz_scale()) etc | 00:28.43 |
| | Robin_Watts: since it reads worse, I'd prefer to see a measurable improvement on arm before going forward with it | 00:29.18 |
| Robin_Watts | tor8: Previously we'd always make a matrix (fz_scale) then concat it with something else fz_concat(fz_scale(), mat); | 00:29.21 |
| sebras | marcosw: you might want to use apt-cache search instead, depending on how you search. | 00:29.28 |
| | unsure if it is faster/slower, but it's an alternative. | 00:29.42 |
| Robin_Watts | I now do: fz_pre_scale(mat, sx, sy); | 00:29.43 |
| tor8 | Robin_Watts: or in lots of places: fz_transform(ctm, fz_bound_something()) | 00:29.49 |
| | did you split up all those? | 00:30.00 |
| Robin_Watts | i.e. 'form a scale matrix, and premultiply mat by it'. | 00:30.04 |
| | concat is now much more rarely used. | 00:30.12 |
| | and consequently we do far fewer full matrix multiplications. | 00:30.23 |
| tor8 | Robin_Watts: right. | 00:30.34 |
| Robin_Watts | Similarly I have fz_{pre,post}_transform | 00:30.42 |
| tor8 | Robin_Watts: ugh. I hate having options there! I always mess up the matrix multiplication order... | 00:31.06 |
| Robin_Watts | tor8: well, it's no worse than having to decide whether to fz_concat(scale, A) or fz_concat(A, scale) | 00:31.37 |
| tor8 | if it makes sense to just have fz_scale(mat, sx, sy) and no fz_scale that returns a matrix that's fine by me | 00:31.52 |
| | where fz_scale would be the equivalent of concatenating the scaling matrix onto the existing one | 00:32.16 |
| | but measure first, please :) | 00:32.21 |
| Robin_Watts | tor8: We can take a look at the patch when I solve the crashes I've just found :( | 00:32.27 |
| | We should measure on ARM. Which means I should either build for beagleboard, or raspberry pi. | 00:33.16 |
| | It'll give me an excuse to plug the pi in :) | 00:33.34 |
| | Neither of those devices are quite as low powered as the customer is using. | 00:34.17 |
| | Hmm. Maybe I should port to RISC OS :) | 00:34.34 |
| tor8 | well, relative measurements should still be valid though? | 00:34.37 |
| Robin_Watts | Depending on the relative cost of FP ops, yes. | 00:34.59 |
| sebras | Robin_Watts: could be worth optimizing for NEON, but you might already have done so..? | 00:49.16 |
| Robin_Watts | sebras: I have not done so. | 00:49.29 |
| sebras | wouldn't help for the pi, but it would for your beagleboard... | 00:49.40 |
| Robin_Watts | I'm not sure NEON would help us much above and beyond normal FP usage. | 00:50.00 |
| | We don't use FP on a per-pixel basis, generally. | 00:50.13 |
| | hence we can't deal with lots at a time. | 00:50.24 |
| | It's entirely possible that the compilers are still pants at writing FP code though. | 00:50.53 |
| sebras | I wouldn't be surprised. | 00:51.06 |
| Robin_Watts | but the first step in any optimisation should be to make sure we're using the best algorithm first. | 00:51.27 |
| sebras | no, that's not the first step. | 00:51.47 |
| | the first step is to measure. ;) | 00:51.52 |
| Robin_Watts | Well, ok :) | 00:52.20 |
| sebras | but it's definitely the 2nd. | 00:52.31 |
| | NEON seems to be able to paralellize integer math too, which we ought to be doing some of when blending..? | 00:53.25 |
| Robin_Watts | Now, that is possible. | 00:53.41 |
| | We have a customer that had taken a previous version of mupdf and optimised it for their low powered embedded ARM systems. | 00:55.04 |
| | and when we talked about their changes, one of the big things was the matrix maths. | 00:55.27 |
| tor8 | Robin_Watts: I wonder what the main bottleneck there was? | 00:57.07 |
| sebras | is it just the copying or the ops themselves that are problematic? | 00:57.12 |
| tor8 | there aren't a lot of fz_scale etc going on I'd believe, for the fz_pre_scale optimization to work | 00:57.30 |
| | ...on | 00:57.40 |
| Robin_Watts | sebras: I suspect it's both. | 00:57.45 |
| | if we have a function that takes an fz_rect as an arg, and passes it on to another function inside, that's 4 loads, and 4 stores on the caller end, then 4 loads and 4 stores on the callee end. | 00:59.00 |
| | by reference it's 1 load/1 store at the caller, and 1 load/1 store at the callee. Unless we drop the number of register sized lumps used for params to 4 or less, in which case it's just 1 load on the caller, and a move in the callee. | 01:00.13 |
| sebras | sure, but that's just the store/load-multiple instructions. | 01:00.14 |
| | the multiplication/division/whateverop might be the main cause... | 01:00.33 |
| | though I haven't seen your customer's changes of course. :) | 01:00.47 |
| Robin_Watts | Nor have I - they were supposed to be passing us stuff back, but it hasn't happened yet. | 01:01.03 |
| | (plus there are stack loading/saving things that might be avoided due to reducing arg count) | 01:03.44 |
| sebras | other arguments that need not be pushed on the stack. true. | 01:04.21 |
| Robin_Watts | 7% of Android MuPDF users are in Sweden, apparently :) | 01:11.23 |
| sebras | well, I asked my colleagues to install the app and test for us. :) | 01:11.49 |
| | also I count at least twice because I have two phones... | 01:12.02 |
| Robin_Watts | :) | 01:12.18 |
| sebras | Robin_Watts: ARM ARM.pdf is nice. I have missed it. | 01:20.39 |
| Robin_Watts | sebras: Alas, they have discontinued it :( | 01:20.55 |
| sebras | why? | 01:21.08 |
| Robin_Watts | They've changed to other documents. | 01:21.22 |
| sebras | is this why they have this useless treeview-centric webpages now? | 01:22.05 |
| Robin_Watts | Yeah, possibly. | 01:22.14 |
| sebras | :-P | 01:22.34 |
| Robin_Watts | ok, I've fixed a bug, and now I'm off to bed before the cluster test finishes and sucks me into the next one :) | 01:31.17 |
| | Night all. | 01:31.20 |
| sebras | g'night. | 01:31.24 |
| ray_laptop | I just closed the sharepoint bug 693600. Like we care that Microsoft can't index files created by us and Distiller (and probably about 90% of the lame-o PDF creators we see) | 02:01.14 |
| | it's probably an encoding issue, and kens has enough (real) issues to deal with | 02:01.55 |
| | bmpcmp came up with a TOTALLY different set of tests compared to what I got from the regression report :-( Trying again | 02:08.43 |
| | OK. Well, not totally different. At least most of the diffs are due to the way I FORCED NumRenderingThreads=4 (99-01-fixed.PS) | 02:18.07 |
| | chrisl_away: (for the logs) You may want to cherry-pick my commit 803a59e : Bug 693590: MT rendering has wrong 'supports_devn' and incomplete color_info | 07:39.36 |
| | chrisl_away: This fixes all serious problems with NumRenderingThreads=4 producing the wrong output (but 693590) | 07:40.23 |
| | bug 693590 | 07:40.33 |
| | the 99-01-fixed issues are spurious due to the way I tested it. There are a couple of CET files that I need to look at, but I think it is good enough as is. | 07:42.22 |
| | chrisl_away: marcosw: We need to run a -dNumRenderingThreads=4 regression to see for sure (the way I forced num_render_threads_requested may be the cause of some of the remaining issues as with 99-01-fixed.PS) | 07:44.16 |
| kens | Another morning another email from Aaron.... | 08:08.18 |
| Robin_Watts | Morning tor8. | 10:05.07 |
| | On windows, a release build 72dpi -o nul: run of mudraw of the first 400 pages of pdf_reference17.pdf saves 1% with the pass by reference stuff. | 10:06.03 |
| sebras | Robin_Watts: hm... do you have my chip-design PDF? | 10:07.01 |
| | Robin_Watts: there are insane amounts of paths there, and so I believe that your patch might give a higher improvement for that file. | 10:07.31 |
| Robin_Watts | I wasn't expecting to see any real improvement on windows; FP and memory are so fast, and the work would be dwarfed by the per pixel stuff, I reckon. | 10:08.42 |
| tor8 | Robin_Watts: yeah, that's more than I expected on windows! reproducible as well, I take it? | 10:09.19 |
| Robin_Watts | I ran each test 5 times. | 10:09.34 |
| | More like .75% for the whole file (all 1310 pages or so). | 10:15.56 |
| | sebras: No, do you have a pointer to it ? | 10:16.03 |
| sebras | Robin_Watts: I sent tor8 looking for it. I have it at home, but my desktop is powered down. | 10:16.50 |
| tor8 | sebras: nope, not in my archive by that name. | 10:18.01 |
| sebras | tor8: :-/ I'll get it for you tonight. | 10:18.15 |
| Robin_Watts | tor8: trivial review: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=d8ad064b0c4bcd539c06c98af070613ff818ee0b | 10:21.39 |
| | Fixes the dependencies in VS. I'm sure I keep fixing it, and then forgetting to commit it. | 10:22.02 |
| tor8 | Robin_Watts: looks fine. | 10:22.13 |
| Robin_Watts | Ta. | 10:22.19 |
| | sebras: Can you remember the name/title of the document? Maybe Mr Google can find it. | 10:27.58 |
| | tor8: Random question... | 11:01.36 |
| | At various points in mupdf we have: | 11:02.00 |
| | if (fabsf(m->b) < FLT_EPSILON && fabsf(m->c) < FLT_EPSILON) | 11:02.02 |
| | FLT_EPSILON: This is the difference between 1 and the smallest floating point number of type float that is greater than 1. It's supposed to be no greater than 1E-5. | 11:03.03 |
| | ok, I guess I've answered my question. The delta between representable float values is smaller around 0 than around 1, right? | 11:04.14 |
| paulgardiner | Yeah, so maybe those tests don't do what they were intended for. | 11:05.10 |
| Robin_Watts | no, I think they do do what they were intended to do. | 11:05.59 |
| | If you get a rounding error in a value that's about 1, you can expect it to be FLT_EPSILON out. | 11:06.19 |
| | If you get a rounding error in a value that's around 0, you can expect it to be smaller than FLT_EPSILON. | 11:06.42 |
| | I was worried that < FLT_EPSILON might actually be equivalent to == 0. | 11:06.57 |
| | but I now don't think it is. | 11:07.15 |
| paulgardiner | Many factors smaller though, wouldn't itbe ? | 11:07.21 |
| tor8 | Robin_Watts: quite. FLT_EPSILON is a magic number drawn out of a hat really | 11:07.49 |
| Robin_Watts | paulgardiner: That's fine. | 11:08.06 |
| | tor8: yeah, it's an arbitrary "small value" for our purposes. | 11:08.16 |
| paulgardiner | Ah right. Fine if it's magic | 11:08.21 |
| Robin_Watts | So in fz_bound_shade... | 11:08.25 |
| | We intersect a rect with the infinite rect. WTF? | 11:08.39 |
| tor8 | basically, that use is a test for if a value is near enough to zero for our purposes | 11:08.52 |
| Robin_Watts | tor8: yes, I'm about to introduce another similar test, I think, hence me staring at it. | 11:09.11 |
| tor8 | Robin_Watts: static inline int fz_is_zero(f) { return fabsf(f) < FLT_EPSILON; } is fine by me if you want | 11:10.00 |
| | then you can remove the call to fabsf and get better performance | 11:10.13 |
| Robin_Watts | tor8: You mean: f < FLT_EPSILON && f > - FLT_EPSILON ? | 11:10.46 |
| tor8 | || but yes | 11:11.32 |
| Robin_Watts | No, &&. | 11:11.43 |
| tor8 | or maybe I should get some coffeine... | 11:11.50 |
| Robin_Watts | I'm caffinating now :) | 11:12.01 |
| | tor8: How would you feel about some macros to cast from a pointer to a float/float paint to a point *? | 11:13.44 |
| | specifically so I can get x0/y0 from a rect as a point without a copy ? | 11:14.02 |
| | In various places we copy x0/y0 into a point, then call a transform on it, then copy it back. | 11:14.24 |
| | The alternative is to do a gs and make fz_rects a pair of fz_points. | 11:15.06 |
| | (but that doesn't help in the path code, which is the other big user of that) | 11:15.20 |
| tor8 | so struct fz_rect { fz_point min, max; } ? | 11:18.18 |
| Robin_Watts | That's what gs does. | 11:18.28 |
| | although gs uses p and q. | 11:18.41 |
| tor8 | I've considered it myself, just haven't bothered | 11:18.43 |
| | I'd prefer min,max or a,b | 11:18.53 |
| Robin_Watts | and point.p.x is nastier than point.x0 | 11:18.59 |
| tor8 | rect.min.x is better than rect.p.x I'll give you that | 11:19.22 |
| | rect.a.x is also better | 11:19.27 |
| Robin_Watts | I'd prefer leaving it as is, and using macros to cast it :) | 11:19.30 |
| tor8 | static inline function, remember that customer who didn't like macros? | 11:19.53 |
| Robin_Watts | oh, right, fair enough. | 11:20.06 |
| tor8 | fz_point fz_rect_min/max(r) | 11:20.32 |
| Robin_Watts | fz_point *fz_rect_{min,max}(fz_rect *r) | 11:21.00 |
| | That's much nicer than what I had in mind. Thanks. | 11:21.24 |
| tor8 | ouch, fz_intersect_bbox modifies one of the two input values? | 11:21.52 |
| Robin_Watts | In my patch, yes. | 11:22.16 |
| tor8 | please, fz_intersect_rect(fz_rect *out, const fz_rect *a, const fz_rect *b) | 11:22.22 |
| Robin_Watts | It intersects the first bbox with the second. | 11:22.28 |
| | All the functions are of the form: modify the first by combining with the second. | 11:22.48 |
| | tor8: The 3 arg formulation involves more copying, exactly what we are trying to avoid. | 11:23.39 |
| tor8 | so you're making the code completely ugly and unreadable :( | 11:24.03 |
| Robin_Watts | And we have to be careful to code carefully when out == a or out == b. | 11:24.07 |
| | I'm not averse to having a 3 arg form, but we shouldn't only have that form. And I couldn't think of a nice naming scheme. | 11:24.56 |
| tor8 | one form is better than a bazillion forms that remain unused | 11:25.21 |
| | also better for code cache | 11:25.32 |
| | I think I can get used to 2-form if it's consistent. I just had a gut reaction there! | 11:25.53 |
| Robin_Watts | phew. | 11:26.46 |
| paulgardiner | :-) | 11:27.09 |
| Robin_Watts | I'm completely open to ways to make the code more readable, as long as it doesn't harm the performance, which was the point of this. | 11:27.11 |
| paulgardiner | I seem to find myself usually wanting f(update_this, with_this) | 11:27.37 |
| | ... when intersecting and unioning rects | 11:28.09 |
| tor8 | Robin_Watts: do you think it would be possible to keep only the fz_pre_xxx matrix functions and do away with the others? | 11:28.20 |
| Robin_Watts | paulgardiner: That's what I've currently got. | 11:28.24 |
| | tor8: Not trivially, sadly. | 11:28.38 |
| tor8 | I like the trick of returning the "out" argument so you can nest them | 11:28.39 |
| Robin_Watts | tor8: On ARM, that works very well, cos you pass the first arg in in r0 (if it's a pointer), and the return value from the function comes out as r0. | 11:29.15 |
| | so it's something you get for free. | 11:29.19 |
| tor8 | Robin_Watts: even better! | 11:29.31 |
| Robin_Watts | hence there are some places where I pass the pointer in as the last arg, and I'd really like them to be the first. | 11:29.58 |
| paulgardiner | ... though probably the copying overhead of the more algebraic approach is minimal unless we are doing a huge number of operations. | 11:30.04 |
| tor8 | like this: fz_scale(fz_rotate(fz_identity(&ctm), 180), -1, -1) | 11:30.18 |
| Robin_Watts | but I'm not sure that fits with our calling scheme. | 11:30.18 |
| tor8 | Robin_Watts: ctx going first messes up a lot | 11:30.46 |
| Robin_Watts | exactly. | 11:30.54 |
| tor8 | I think fz_identity should be a function that initializes a matrix if we're going pass-by-reference | 11:31.50 |
| Robin_Watts | I like that a lot. | 11:32.22 |
| tor8 | or have two functions, one that returns a "global" no-touch identity matrix | 11:32.39 |
| Robin_Watts | Possibly also: fz_matrix_copy(&dst, &src) | 11:32.43 |
| paulgardiner | I'd better hurry up with the annotations: no way I want to try to rebase through this change your planning. :-) | 11:32.45 |
| tor8 | const, of course :) | 11:32.45 |
| Robin_Watts | paulgardiner: I have to massively rebase on top of tor8's commits. | 11:33.08 |
| | so I'm resigned to that. | 11:33.16 |
| | paulgardiner: If you want to get some preparatory reviews in first, you have lots of time. | 11:33.30 |
| paulgardiner | Is there anywhere in mupdf where we do sufficient rect operations that the copy overhead is significant? | 11:34.50 |
| Robin_Watts | In tests this morning, I saved 1% on the first 400 pages of pdf_reference17.pdf on windows. | 11:35.32 |
| | I suspect that ARM will be more sensitive to this. | 11:35.51 |
| paulgardiner | Consistently on repeated tests? | 11:36.01 |
| Robin_Watts | Yes. | 11:36.07 |
| paulgardiner | Oh ok. | 11:36.26 |
| Robin_Watts | In particular we have a customer who heavily optimised a previous version of mupdf for low power ARMs, and they identified the matrix stuff as being a heavy user. | 11:36.39 |
| paulgardiner | One pain with the reference method is that expressions like (tr1(a /\b))\/(tr2(c/\d)) require declaration of vars for each subexpression | 11:38.33 |
| | but then I guess we don't often require complicated expressions | 11:38.47 |
| Robin_Watts | paulgardiner: Yes, that is a pain, but I think we can minimise that. | 11:39.19 |
| | I will update my patch with fz_identity etc and see if I can figure out if we can remove some fz_post_xxxx's. | 11:39.55 |
| | Removing posts is not possible. | 11:59.15 |
| tor8 | Robin_Watts: where do we need them? | 12:02.23 |
| Robin_Watts | EVERYWHERE :) | 12:02.31 |
| tor8 | my guess would be patterns only | 12:02.36 |
| Robin_Watts | pdfapp_viewctm | 12:02.51 |
| | pdf_form.c: account_for_rot | 12:03.30 |
| tor8 | pdfapp_viewctm shouldn't need it. | 12:03.55 |
| Robin_Watts | fz_post_scale(fz_rotate(mat, app->rotate), app->resolution/72.0f, app->resolution/72.0f); | 12:04.25 |
| tor8 | return fz_scale(fz_rotate(fz_identity(&ctm), app->rotate), app->resolution/72.0f); | 12:04.27 |
| | then fz_rotate(fz_scale(...)) ? | 12:04.43 |
| Robin_Watts | ok. | 12:05.22 |
| tor8 | reordering function calls is better, no? | 12:05.36 |
| Robin_Watts | I am fairly sure we'll still have cases where that's not enough. | 12:06.11 |
| | but I will try. | 12:06.14 |
| tor8 | in which case we can take two matrixes and concat in whichever order with fz_concat, I'd imagine | 12:06.34 |
| Robin_Watts | The moment we do an fz_concat, we've cost ourselves copying. | 12:07.13 |
| | hmm. | 12:07.46 |
| | I haven't written fz_{pre,post}_rotate yet. | 12:07.59 |
| tor8 | Robin_Watts: in the one rare occurence where I think we may have to, I don't think that matters | 12:08.04 |
| | (i.e. the pattern tiling space setup in the pdf interpreter) | 12:08.16 |
| Robin_Watts | I'll give it a go. | 12:08.20 |
| | In pdf_load_page, I think we need it there. | 12:09.27 |
| | fz_post_scale(fz_rotate(&page->ctm, -page->rotate), 1, -1); | 12:10.26 |
| | realbox = page->mediabox; | 12:10.28 |
| | fz_transform_rect(&page->ctm, &realbox); | 12:10.30 |
| | fz_post_translate(fz_post_scale(&page->ctm, userunit, userunit), -realbox.x0, -realbox.y0); | 12:10.32 |
| | The first one we can fix. | 12:10.34 |
| | The latter ones, I can't see how. | 12:10.44 |
| | And there is a case in mudraw. | 12:12.00 |
| tor8 | neither places which are performance critical in any way at all. live with the extra copy there. | 12:15.23 |
| Robin_Watts | ok. | 12:16.00 |
| | OK, fz_post_.... expunged. | 12:40.52 |
| tor8 | Robin_Watts: did it end up alright? | 12:53.17 |
| Robin_Watts | tor8: Yes. You were right. | 12:53.48 |
| | I will test and repush in a mo. | 12:54.02 |
| tor8 | Robin_Watts: updated commits on master. I could squash those into one if that would be cleaner to read. | 13:00.42 |
| | tor/master that is | 13:00.52 |
| Robin_Watts | tor8: fab. I will look straight after lunch. | 13:03.17 |
| | tor8: Would you mind squeezing them into one? Less chance of me missing stuff, I think. | 13:52.43 |
| tor8 | try tor/squished | 13:54.39 |
| Robin_Watts | The android stuff is calling fz_clear_pixmap_rect_with_value(.... abox) | 13:58.23 |
| | where abox is a rect, not an irect ? | 13:58.32 |
| | jni/mupdf.c line 811ish | 13:59.07 |
| tor8 | Robin_Watts: right, I still need to fix the android for the irect... | 13:59.08 |
| | and probably the ios app too. argh. knew there was something I forgot. | 13:59.26 |
| Robin_Watts | I haven't tried building android etc with my reference changes yet. Figure that can wait. | 13:59.50 |
| chrisl | Another memory bug bites the dust..... | 14:05.19 |
| Robin_Watts | chrisl: The gc one? | 14:05.30 |
| | nice. | 14:05.32 |
| chrisl | Yeh, really surprised that hasn't happened more often | 14:05.49 |
| Robin_Watts | tor8: So you've changed 'area' in the fz_draw_state structure from a rect to an irect. | 14:20.31 |
| tor8 | Robin_Watts: yeah, I wasn't sure what to do with that one. the places where we passed it further on was an irect. | 14:20.56 |
| Robin_Watts | If that's a deliberate change, then great. | 14:21.33 |
| paulgardiner | Robin_Watts, tor8: The strikeout annotation commis are on paulg/master. One of them has API additions that you both probably should check. Also I need to work out what to do about some functions from fitz-internal.h that I'm using. | 14:22.26 |
| Robin_Watts | paulgardiner: I don't think using fitz-internal.h functions in the app is a problem. | 14:23.42 |
| | The main thing about fitz-internal.h stuff is that we might change it. | 14:23.57 |
| | (not that we'd ever change anything in fitz.h, oh no...) | 14:24.09 |
| paulgardiner | :-) | 14:24.17 |
| | Hmm. The API for adding appearance streams isn't a lot of use without ways to build a display-list so I may be forcing others to use internal | 14:25.06 |
| Robin_Watts | Put the appearance streams API in fitz-internal.h too then ? | 14:25.30 |
| paulgardiner | Oh maybe | 14:25.41 |
| | ... although if we say we support annotation creation maybe it should be possible via public interfaces. | 14:26.25 |
| | ... but I'm happy to go with your suggestion. | 14:26.43 |
| Robin_Watts | fitz-internal.h is "public", just not "frozen" | 14:26.56 |
| tor8 | Robin_Watts: maybe after next release we should do another attempt at reorganizing the headers. | 14:27.15 |
| paulgardiner | Right | 14:27.18 |
| Robin_Watts | tor8: Maybe. | 14:27.26 |
| | tor8: OK, the rest of that patch looks good to me. | 14:44.52 |
| | A couple of comments... | 14:45.06 |
| | 1) The patch would be a lot less bulky if we replaced fz_irect with fz_bbox. | 14:45.34 |
| | fz_irect is a nicer name, I think, but fz_bbox is something that pretty much every caller of our code will be used to. | 14:46.03 |
| | 2) In the 'pass by value' thing, we lose the ability to pass NULL for rects and instead have to pass fz_infinite_rect | 14:46.51 |
| | I'll have to remember to put that back in the pass by reference thing. | 14:47.26 |
| | paulgardiner: The pdf_device commit looks good to me :) | 14:51.37 |
| paulgardiner | Really! That was the one I was least proud of. :-) | 14:51.59 |
| Robin_Watts | ha! | 14:52.05 |
| | And the improve exception handling looks fine. | 14:53.54 |
| | Add support for annotation creation lgtm | 15:00.24 |
| | Generally 'GetFieldID' operations are considered to be slow. | 15:02.57 |
| | Possibly we should have a 'getFieldIDs' function that is called once when MuPDFCore starts up, and gets all the field IDs we'll ever need into static variables. | 15:03.33 |
| | That way we could avoid doing them every time we do a strikeout. | 15:03.43 |
| | Otherwise that review lgtm too. | 15:05.05 |
| | paulgardiner: In the 'only use one fz_page object' one... | 15:08.05 |
| | In drawPage, you update_changed_rects, which makes both lists. Then you drop one list. | 15:08.36 |
| | Why not just make one list in the first place ? | 15:08.45 |
| paulgardiner | Just saved a little complication... extra parameters and stuff, but yeah I was wondering whether to change that | 15:10.11 |
| | Robin_Watts: actually I have to drop the list there anyway because it might already contain rects | 15:13.51 |
| | ... but obviously I may be adding extra before droping the way I'm doing it now. | 15:14.17 |
| Robin_Watts | ok. It might be nicer to just build the one list. but that review looks fine to me too. | 15:15.32 |
| | as does the last one. | 15:15.53 |
| | so, tor8, do you want to cast your eye over those reviews too? | 15:16.30 |
| | Now we need to have a fight to see which of you gets to commit first. | 15:16.51 |
| Robin_Watts | fetches tea to spectate. | 15:17.10 |
| paulgardiner | I bet the one of us that has access to the main repo wins. :-) | 15:18.11 |
| Robin_Watts | I don't think it will be a huge job to rebase one on top of the other. | 15:26.58 |
| paulgardiner | I'm easy either way. | 15:28.59 |
| | tor8: is yours multiple commits too? | 15:29.44 |
| chrisl | kens: ping | 15:38.03 |
| Robin_Watts | paulgardiner: it is, or it isn't, depending on whether we take the squished one or not. | 15:38.23 |
| kens | chrisl pong | 15:40.35 |
| chrisl | Have you looked at the -Z@ regression run? | 15:40.49 |
| kens | No, not at all | 15:40.56 |
| | Why ? | 15:41.05 |
| | I have a bug report from Alex with -Z@ in teh command line | 15:41.29 |
| chrisl | "New segfaults: (9930)" - all seem to be with pdfwrite or ps2write, but I'm getting crashes at least back to 9.06 | 15:41.59 |
| kens | probably all that memory work | 15:50.23 |
| | If marcos doens't tell me.... | 15:50.39 |
| chrisl | kens: these really aren't "new" segfaults, as far as I can tell - I have no idea why they've only just popped up | 15:50.43 |
| kens | Hmm, odd, maybe Marcos will know | 15:52.45 |
| chrisl | kens: do you want a bug opened about it? | 15:54.39 |
| kens | Not really, but if you think it warrants it | 15:54.52 |
| chrisl | Well, pdfwrite appears to be using memory that's been freed, that's not really a good thing | 15:55.23 |
| paulgardiner | I'm going see how the rebase goes | 15:55.59 |
| Robin_Watts | paulgardiner: tor8's review is not finalised yet. | 15:57.38 |
| paulgardiner | Oh okay. I wont then | 15:57.59 |
| tor8 | I'm still fixing | 15:58.05 |
| | give me a few mins | 15:58.16 |
| | okay, check tor/squished | 15:59.50 |
| | Robin_Watts: fz_bbox named version of the squished patch on tor/master | 16:01.35 |
| | probably best not to change the name even if irect is better, to maintain some semblance of api stability :) | 16:01.51 |
| chrisl | kens: yes, it was 19e1c90a1 - "pdfwrite - address memory leaks" when it started :-( | 16:02.01 |
| Robin_Watts | tor8: And the android and ios builds both work? | 16:02.43 |
| kens | :-( | 16:02.54 |
| | Its probably the same problem Alex reported | 16:03.06 |
| chrisl | The file I looked at, the crash happens freeing "pdfont->BaseFont.data" in font_resource_free(). | 16:04.10 |
| kens | that's not the same then :-( | 16:04.48 |
| tor8 | Robin_Watts: ah, probably not... | 16:05.30 |
| | I don't care about the ios build, I can fix that later | 16:05.48 |
| Robin_Watts | want me to fix the android build, or are you set up for that? | 16:06.12 |
| tor8 | it would be easier if you fixed it, I think | 16:06.59 |
| | it always takes me a while to get back into building the android app | 16:07.33 |
| | (I have to use macosx for that, not linux, since my linux doesn't have the usb debugging set up, living in a virtual machine as it is) | 16:08.06 |
| marcosw | chrisl: are there binaries of the 9.07 release candidate? If so I was going to let Gemma know, she seems to actually test the releases. | 16:08.19 |
| Robin_Watts | I'm looking at it now. | 16:08.21 |
| chrisl | marcosw: there isn't a release candidate yet - I was under the impression we didn't want a candidate until your testing was (nearly?) done | 16:09.03 |
| marcosw | chrisl: I didn't realize that, I thought regression testing and testing by users/customers would be contemporaneous. | 16:10.42 |
| chrisl | marcosw: Okay, I can do an rc tomorrow. I don't normally do binaries, but I can make an exception this time | 16:11.26 |
| | marcosw: so have you any idea why all those pdfwrite/ps2write segfaults with -Z@ suddenly appeared today in the regression report? | 16:14.39 |
| marcosw | chrisl: haven't looked yet, but it is worrying... | 16:15.04 |
| | chrisl: at first glance they appear real. I'll confirm and bisect; maybe we will get lucky and the offending commit won't be in the 907 branch :-) | 16:19.29 |
| chrisl | marcosw: they are real, they started with commit 19e1c90a1 - Apr 19 2012 | 16:20.35 |
| marcosw | chrisl: right, there was a bug in my regression script :-( The -Z@ option wasn't being passed to the pdfwrite/ps2write step. I fixed that a few days ago... | 16:24.45 |
| | so we just need to document that -Z@ and pdfwrite/ps2write is not allowed :-) | 16:25.10 |
| chrisl | marcosw: ah, that explains it. Given that it's been out there like this for 6 months already, I don't see it as a blocker | 16:25.31 |
| marcosw | I agree. But perhaps I'll open a bug... | 16:26.09 |
| Robin_Watts | tor8: If you're happy with this, http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=c5bb011a389d73b95f42684356fdeca6708b8c71 I'll push. | 16:29.16 |
| | Then I'll try rebasing pauls stuff onto it. | 16:29.26 |
| tor8 | that looks fine. | 16:30.32 |
| paulgardiner | Robin_Watts: don't worry. I'll do it. It needs little tweaks at each stage because some bits merge ok but then don't build. | 16:30.43 |
| Robin_Watts | paulgardiner: OK, but if you run out of time tonight, please say, so I can continue with it. I have to rebase my changes onto the end of both of yours... | 16:31.53 |
| paulgardiner | So I'm aiming at robin/torfix? | 16:33.25 |
| Robin_Watts | golden mater | 16:34.27 |
| | golden master | 16:34.32 |
| paulgardiner | ah yes | 16:35.09 |
| chrisl | kens: (sorry for poking my nose in!) The fix is trivial, I think - if you're interested...... | 16:40.20 |
| kens | chrisl, yes I'm interested, sorry was fetching coffee | 16:41.03 |
| chrisl | here: http://git.ghostscript.com/?p=user/chrisl/ghostpdl.git;a=commitdiff;h=14c30543 | 16:41.18 |
| kens | Seems reasonable, I didn't realise the base14 were handled differently | 16:42.20 |
| | Oh its the name, fair enough then | 16:42.48 |
| chrisl | So, I'll clusterpush that, if it comes back good then commit it? | 16:43.15 |
| kens | Absolutely, makes perfectly good sense | 16:43.27 |
| | standard names are held 'differently' | 16:43.54 |
| | I guess that won't fix all of the problems though | 16:44.14 |
| chrisl | Good, I think I'll stop looking at the weekly regression mails - I only looked to double check my fix from last week! | 16:44.14 |
| kens | I kind of rely on Marcos to tell me if I've screwed anythign up there | 16:44.34 |
| chrisl | Well, at worse, hopefully, that'll get us down to manageable numbers | 16:45.18 |
| kens | Shold get rid of a lot I htink | 16:45.29 |
| chrisl | Ah, cool, my push got promoted ahead of a mupdf torrent... :-) | 16:46.25 |
| Robin_Watts | user pushes should, yes. | 16:46.48 |
| | but mupdf cluster tests are much faster than gs ones anyway. | 16:47.13 |
| chrisl | Yeh, at first glance I didn't realise they commit runs | 16:47.25 |
| kens | And another email from Aaron, I'm going to run away now before he thinks of some more questions. Goodnight all | 17:07.09 |
| | <sigh> too late, there's the next one.... | 17:07.20 |
| paulgardiner | Robin_Watts: the rebase built but now partial update is broken, so maybe best if you commit your stuff and rebae over that tomorrow. Possibly partial update is broken on master: I haven't tested it. | 17:07.27 |
| Robin_Watts | paulgardiner: I'm not ready to commit just yet. | 17:07.53 |
| | I'm rebasing over tors stuff at the moment. | 17:08.07 |
| | so that will take me a while. | 17:08.16 |
| | An apk built from master runs calc.pdf fine. | 17:08.52 |
| kens | Can someone else answer the odd email to support about PDF->EPS please? I need to go have a pizza. | 17:09.14 |
| Robin_Watts | and the W9 form fills in correctly. | 17:09.16 |
| paulgardiner | Ta. saves me the test | 17:09.25 |
| | Robin_Watts: nah. I can't find it and I'm out later | 17:23.13 |
| Robin_Watts | paulgardiner: No worries. | 17:25.33 |
| paulgardiner | Robin_Watts: it's okay up to the single page stuff. I've pushed the good commits to paulg/strike | 17:31.38 |
| Robin_Watts | paulgardiner: Ah, ok, I'll look at them later and if tor8 is happy with them, push them to golden. | 17:32.26 |
| paulgardiner | Thanks. And then I can rebase the last two over your stuff when it's done | 17:33.01 |
| Robin_Watts | I doubt my stuff will be ready before yours. | 17:33.27 |
| henrys | wonder if we should check in with this fellow http://bugs.ghostscript.com/show_bug.cgi?id=692947 now that the new phones are out and headlines. | 18:01.03 |
| tor8 | Robin_Watts: apparently "Visual Studio Tools for Git" released today | 18:02.38 |
| Robin_Watts | tor8: I've had an old version of that installed for a while. | 18:03.03 |
| | henrys: fundamentally our position hasn't changed though. | 18:05.12 |
| henrys | no but I'd like to know the status of his port in case we get inquiries. | 18:06.32 |
| Robin_Watts | https://github.com/llmike/PhMuPDF/ | 18:07.20 |
| | source and binaries. | 18:07.25 |
| | oh, no source. | 18:08.08 |
| henrys | looks dead | 18:08.13 |
| Robin_Watts | In which case he's in violation of the GPL. | 18:08.14 |
| henrys | you pulled the git and got a binary with no source? | 18:09.54 |
| Robin_Watts | I browsed the git, and all there is there is a README. | 18:10.08 |
| | On the downloads page however there is a binary. | 18:10.15 |
| ray_laptop | I don't do 32-bit builds on linux often enough. Can anyone (alexcher ?) tell me how to check if my bin/gs is 32-bit ? | 18:13.39 |
| Robin_Watts | file bin/gs ? | 18:13.55 |
| ray_laptop | Robin_Watts: ahh, thanks. I didn't realize "file" was smart enough to give all that info | 18:14.30 |
| alexcher | yes, what exactly should I so? | 18:14.48 |
| ray_laptop | and I _do_ have a 32-bit build | 18:14.54 |
| | alexcher: thanks. Robin_Watts answered me. | 18:15.18 |
| henrys | I am looking at the download page and see a bin.tar and src.tar the latter seems to contain his source changes. Don't see a problem. | 18:17.17 |
| Robin_Watts | Ah, so he's just abusing github, fair enough. | 18:20.09 |
| henrys | this phone is getting some notice I wonder how prepared we should be for requests. I know you guys didn't want to maintain another port, but... | 18:22.36 |
| ray_laptop | darn bug 693592 only fails on linux 32-bit. Windows works fine. I guess that is good, however, since 32-bit linux is more rare nowadays. | 18:23.37 |
| Robin_Watts | henrys: It's not that I don't want to maintain another port, it's that none of us are currently set up to even build such a port. | 18:24.04 |
| | If we decide we want to take that step, then we can do it - it's just not a 'zero effort' thing to take his patches on. | 18:24.28 |
| henrys | understood - what I meant by maintain a port is buy a device, set up the devel env. etc. | 18:25.42 |
| | next meeting fodder | 18:26.13 |
| Robin_Watts | yeah. | 18:26.34 |
| alexcher | ray_laptop: there was a bug on 32-bit platforms fixed recently. It's worth to retest. | 18:27.15 |
| ray_laptop | alexcher: i just tested it with commit http://git.ghostscript.com/?p=ghostpdl.git;a=commit;h=9c11bda | 18:28.16 |
| | alexcher: when it worked on Windoze, I was hoping it had been fixed, but it still fails on peeves :-( (no warnings like Marcos reports, but missing data on the image) | 18:29.51 |
| henrys | valgrind is the same? | 18:32.13 |
| ray_laptop | ahh. On peeves with a debug build I get FPE during rendering. Still not identical to what Marcos reports, but enough to go on | 18:39.54 |
| paulgardiner | Robin_Watts: found it. paulg/strike now has the full set of commits | 18:41.08 |
| Robin_Watts | paulgardiner: Ah, fab. | 18:41.26 |
| | I'm happy with the commits there, but I'd like to get a nod from tor8 before I push them. | 18:41.52 |
| marcosw | chrisl_away: sorry about the cluster problems, I working on it. | 18:59.44 |
| tor8 | Robin_Watts: paulgardiner: we haven't used the verb "make" anywhere else, any reason why it's not fz_new_annot? (or fz_create_annot to mirror pdf_create_object) | 19:04.33 |
| | (or maybe that one ought to be renamed as well) | 19:04.44 |
| Robin_Watts | You're probably right. | 19:05.07 |
| | I've just rebased my patch over the top of yours. | 19:05.28 |
| | And I'm now unhappy with the ordering I've chosen for some args. | 19:05.38 |
| | C standard is blah(dst, src) | 19:05.47 |
| | and I have fz_round_rect(const fz_rect *rect, bbox *b); which seems wrong. | 19:06.06 |
| tor8 | that should definitely be fz_round_rect(bbox, const rect) | 19:06.29 |
| Robin_Watts | So, I think I'm going to swap everything that way round. | 19:06.38 |
| | Fab. I'm glad you agree. | 19:06.44 |
| tor8 | Robin_Watts: I've been mulling over what you said earlier about first arguments to functions | 19:07.27 |
| | in a perverted way it would make sense to have f(out, ctx, args) order | 19:07.47 |
| Robin_Watts | For the case where we have: fz_blah(context, something, something, out); | 19:07.56 |
| tor8 | rather than f(ctx, out, args) | 19:07.59 |
| Robin_Watts | I think I'm inclined to agree with you. | 19:08.11 |
| tor8 | or worse f(ctx, args, out) | 19:08.13 |
| Robin_Watts | At the moment we have the worst one. | 19:08.25 |
| | I'll change it to the best one. | 19:08.35 |
| tor8 | sometimes we have two output arguments | 19:09.01 |
| Robin_Watts | bah(out1, out2, ctx, ....) | 19:09.14 |
| tor8 | either: int f(out, args) where we return for instance the length of the out argument | 19:09.28 |
| | yes, that's what it should be | 19:09.41 |
| | apart from the "make" verb I'm happy with the (minor) api additions | 19:10.45 |
| | haven't looked at the code, but if robin has reviewed that I'm happy to let it in. once the name has been changed. | 19:11.09 |
| paulgardiner | tor8: fz_make_annot produces something you don't free. Wasn't sure what the conventions was for that. | 19:12.18 |
| tor8 | paulgardiner: I'm not sure we have one. create is closest I think. | 19:13.08 |
| paulgardiner | create sounds good, but I don't have time to change it this evening. | 19:13.57 |
| Robin_Watts | paulgardiner: Either I'll change it, or you can tomorrow. No hurry. | 19:14.15 |
| henrys | uh oh not reaching bugzilla | 20:26.26 |
| | okay now it's back np | 20:27.57 |
| tkamppeter | henrys, why can't we put the fix of bug 692914 into 9.07? I think this is important for printing in Linux distributions. | 20:31.22 |
| henrys | how would I know it is important? to me it is a p4 bug fixed right before the release which is criteria to hold off. | 20:33.57 |
| | I'll commit the code to trunk and it will be up to others to cherry pick | 20:34.41 |
| | tkamppeter: chrisl is the picker so send him mail if you really want it. | 20:35.43 |
| tkamppeter | henrys, thanks. I have mailed to chrisl now. | 20:45.07 |
| henrys | tkamppeter: if you think something is important and a note to the bug and it will get different handling | 20:45.44 |
| | s/and/add | 20:45.53 |
| tkamppeter | henrys, I think we should take the patch for bug 693030 into 9.07, too as it is clearly a bug in terms of PostScript and can also cause complaints of users printing under Linux. | 21:54.47 |
| henrys | I am happy to check it into trunk I certainly could understand if chris wouldn't take it. This is clearly a broken model we enter freeze and you ask for changes, how are we going to fix this? | 21:56.52 |
| | tkamppeter: have to clear that one with with Scott and Miles I don't know when it will be available. | 22:05.48 |
| tkamppeter | henrys, probably I will carry it as distro patch in Ubuntu then. | 22:08.33 |
| henrys | it's a shame because if you just checked in with us 2 weeks ago this wouldn't be an issue. | 22:09.17 |
| tkamppeter | henrys, I did not see this bug, I got only aware of it today as Hin-Tak told me about it. | 22:41.15 |
| henrys | I see well I've gotten the okay so it may be alright. It just seems if we had a process in place for you to review bugs say a month before release we could do a better job | 22:44.01 |
| | Forward 1 day (to 2013/01/31)>>> | |
IRC Logs