| | <<<Back 1 day (to 2013/03/03) | 2013/03/04 |
| henrys | wow 4? I'd never clean up my hard drive with that. | 00:00.41 |
| ray_laptop | Robin_Watts: I found some more problems with clist writing/reading for the copy_planes stuff. VERY obscure depending on when the writing needs to do cmd_write_buffer and when the reading hits the end of the cbuf | 00:35.03 |
| | Robin_Watts: Not blaming you... this stuff is quite fragile and has LOTS of undocumented assumptions, but I think the copy planes is the first place where the elements being written to the clist where not 'atomic' in that a problem developed if the parts were not written and read atomically | 00:37.30 |
| | Robin_Watts: (once again, for the logs)... We can discuss my changes when we are both online tomorrow. | 00:38.32 |
| Robin_Watts | Morning tor8 | 11:34.49 |
| tor8 | hey | 11:35.38 |
| Robin_Watts | Where were we with the last commits on robin/master ? | 11:36.10 |
| paulgardiner | morning | 11:36.19 |
| Robin_Watts | (You may have said something while I was away copying HDs etc) | 11:36.31 |
| | Morning paulgardiner | 11:36.38 |
| paulgardiner | There's an anti-bloat commit on paulg/master. I haven't built under Linux and looked at sizes, but mutool builds on windows without the inclusion of pdf_font.c, so should be okay | 11:38.09 |
| tor8 | Robin_Watts: utf-8 filename patch was okay. the password one I said I'd fix properly, so whether you want to push it now or wait is up to you to decide. I guess I should do it now before I forget, not like I'm going to be able to get back up to speed to do anything meaningful with the gtk viewer before the meeting next weekend | 11:40.03 |
| Robin_Watts | Either we should fix it properly now, or we should push the stopgap. | 11:40.35 |
| | If you're going to fix it properly now, then that's fab! | 11:40.53 |
| tor8 | Robin_Watts: shouldn't be too hard. I just need some proper test files... none are mentioned in the bug report so I guess I'll have to go hunting unless you have a list somewhere | 11:42.26 |
| Robin_Watts | I don't. | 11:43.01 |
| | Can we generate some? | 11:43.15 |
| | How much control does Acrobat give us? | 11:43.38 |
| | My skypey MuPDF company have just reported a bug in 1.2. | 11:44.10 |
| tor8 | I don't have acrobat... | 11:44.18 |
| | (and before anyone pipes up and says buy it, I don't *want* acrobat...) | 11:44.41 |
| Robin_Watts | They render the top and bottom of a page in 2 different threads, and the image that spans the middle doesn't match up, but the text does. | 11:44.47 |
| | I have acrobat. Let me see what I can produce. | 11:44.54 |
| tor8 | Robin_Watts: try with a password containing both a letter in the pdfdoc encoding and latin-1 (say ä) and one that doesn't (like ã
ã
Âã
) | 11:46.12 |
| Robin_Watts | paulgardiner: That anti-bloat commit looks good to me. Does the Android build still build ? | 11:46.17 |
| paulgardiner | Yep | 11:46.28 |
| Robin_Watts | Then I'm happy. tor8 do you want to look or shall I just push? | 11:46.56 |
| | pushed anyway :) | 11:49.28 |
| paulgardiner | ta | 11:50.44 |
| tor8 | Robin_Watts: just push :) | 11:50.52 |
| | Robin_Watts: ugh. SASLprep is supposed to be used to "sanitise" password input | 11:53.56 |
| paulgardiner | I'm now wondering about changing things so that access to annotations is via fz_interactive... also the function to draw only the main contents of the page. | 11:54.12 |
| | Do we still think that is a good change? | 11:54.25 |
| Robin_Watts | Are we ever going to meet another format that has annotations (or something like them) ? | 11:55.07 |
| tor8 | paulgardiner: fz_run_page draws everything and then separate functions for pdf_run_page_main, pdf_run_page_annots? | 11:55.12 |
| paulgardiner | One thought was that we didn't want to force people to separately draw the contents and the annotations just for the sake of drawing the page, but we do already have fz_run_page that does that. | 11:55.16 |
| | tor8: yes :-) | 11:55.29 |
| Robin_Watts | tor8: That sounds good to me. | 11:55.34 |
| paulgardiner | Yes, one of our reasons for wishing to move annotations into fz_interactive doesn't really hold because we already have the single draw function. | 11:56.26 |
| | On the plus side, I could remove several virtual functions from fz_document | 11:57.22 |
| | ... because we'd need only one draw method there, and all the annotation functions could be removed too. | 11:58.05 |
| | We'd also get rid of the strangeness that enumerating widgets is via fz_interactive, but annotations not. | 11:58.34 |
| Robin_Watts | The only reason for NOT removing the annotations is if we can think of a credible reason why we might need them there. | 11:58.38 |
| paulgardiner | On the other hand, I don't see it as that bad how it is. | 11:58.48 |
| Robin_Watts | and that would require us to think of another format that might use them in future. | 11:59.02 |
| paulgardiner | True. | 11:59.08 |
| | And if that were to happen, we could always virtualise fz_interactive | 11:59.30 |
| Robin_Watts | That's very true! | 11:59.42 |
| paulgardiner | Or move functions back into fz_document | 11:59.45 |
| Robin_Watts | I like the idea that interactive things would be inside fz_interactive. | 12:00.02 |
| | so I guess I'm in favour of removing them from standard fz then. | 12:00.17 |
| paulgardiner | I could give it a try, and let you both have a look. We can dump it if we don't like it. | 12:01.19 |
| Robin_Watts | tor8: What character is ã
? | 12:02.17 |
| paulgardiner | It can also be seen as possibly a step towards droping fz_interactive and casting to pdf_document if ever we wish to go that far | 12:02.23 |
| | tor8: worth a try, do you think? | 12:02.55 |
| Robin_Watts | tor8: It won't let me set 'ã
' as a char in the password (for Acrobat 3 compatibility mode at least) | 12:04.37 |
| | Ok. So I need to make it Acrobat 9 or above before it will let me use ã
| 12:10.06 |
| | tor8: http://ghostscript.com/~robin/Tigers.zip | 12:11.19 |
| | Acrobat 9 is BaseVersion 1.7 ExtensionLevel 3 | 12:12.58 |
| | Acrobat 3: /Filter/Standard/Length 40/P -4/R 2/V 1 | 12:14.07 |
| | Acrobat 5: /Filter/Standard/Length 128/P -1028/R 3/V 2 | 12:14.46 |
| | Acrobat 6: /Filter/Standard/Length 128/P -1028/R 4/StmF/StdCF/StrF/StdCF/V 4 | 12:15.46 |
| | Acrobat 7: /Filter/Standard/Length 128/P -1028/R 4/StmF/StdCF/StrF/StdCF/V 4 | 12:16.45 |
| tor8 | funny, zip doesn't seem to cope well with encodings either! | 12:19.01 |
| Robin_Watts | Acrobat 6 is 128bit RC4 | 12:20.15 |
| | Acrobat 7 is 128bit AES | 12:20.21 |
| | Passwords are äääää and ã
ã
ã
ã
ã
| 12:20.48 |
| | All but one the first. | 12:20.55 |
| | tor8: I can try other passwords etc if that helps. | 12:31.04 |
| tor8 | Robin_Watts: I can't open the Acrobat 9 one no matter what I try | 12:39.31 |
| | the ã
ã
Âã
ã
ã
 password messes up everything it seems | 12:39.48 |
| | I can't open it in sumatra on windows either | 12:40.08 |
| | and Zip has created a file with the name "tigerAcr9PASSWORDpaipaipaipaipai.pdf" with accented a and i | 12:40.48 |
| Robin_Watts | opens for me in acrobat reader. | 12:40.53 |
| | yeah, I suspect that zip has utf8 encoded it or something? | 12:41.20 |
| tor8 | why, oh why, is it so hard for everyone to just support UTF8 without doing something stupid? | 12:41.22 |
| | Robin_Watts: looking inside the zip file with a hex dumper, it looks latin-1 encoded | 12:41.43 |
| Robin_Watts | I'm running acrobat reader X here. | 12:41.54 |
| | Acrobat 9 pro opens it too. | 12:42.27 |
| tor8 | Robin_Watts: annoying this is. I can't open it at all if I pass in utf-8 as-is. | 12:47.04 |
| Robin_Watts | Can you open it in Acrobat X reader under windows ? | 12:47.28 |
| tor8 | Robin_Watts: what happens if you make one with the annoying ã
ã
Âã
ã
ã
 password with R=4 | 12:47.34 |
| Robin_Watts | I can't. | 12:47.43 |
| tor8 | it complains? | 12:47.55 |
| Robin_Watts | Acrobat only lets me enter that char into the password box if I set compatibility to Acrobat 9. | 12:48.13 |
| tor8 | Robin_Watts: good to know. | 12:48.37 |
| | btw, ä in pdf doc encoding has the same code point as in latin-1 | 12:48.59 |
| Robin_Watts | Do you want to give me a char that DOESN'T have the same encoding, but is still in the 0-255 range ? | 12:49.30 |
| tor8 | Robin_Watts: that would be good to test against | 12:49.43 |
| | huh. so on windows, unpacking the ZIP file gives the äääää the correct filename but borks ã
ã
Âã
ã
ã
 | 12:50.16 |
| | the opposite on mac, the ä in the filename gets turned into à | 12:50.32 |
| Robin_Watts | I suspect it depends what zipper you use. | 12:50.36 |
| tor8 | and on linux, plain old "unzip" complains in the ã
ã
Âã
ã
ã
 case about mismatching "local" and "central" filenames | 12:51.14 |
| | which suggest your zip tool is also borked somehow | 12:51.22 |
| Robin_Watts | How about char 0xFF. ÿ ? | 12:51.46 |
| | Is that the same in pdfencoding and latin 1? | 12:51.57 |
| tor8 | that's the same in docenc and latin1 | 12:52.23 |
| | *looking through the table* | 12:52.30 |
| | everything from 0xA1 up is the same as latin-1 | 12:53.30 |
| | the euro sign differs | 12:53.46 |
| Robin_Watts | euro being? | 12:54.00 |
| tor8 | and latin small ligature oe | 12:54.16 |
| | ⢠differs too | 12:54.28 |
| | ⬠| 12:56.15 |
| Robin_Watts | I meant, what number is the euro sign? | 12:57.13 |
| tor8 | 0xA0 in pdfdocenc, U+20AC in unicode | 12:57.52 |
| Robin_Watts | It won't let me paste a euro sign into Acrobat until we hit Acrobat 9. | 12:59.41 |
| | Let me try copying a non-break space, which is 0xA0 here. | 13:00.02 |
| tor8 | that's pretty daft, but okay. I guess it may be limiting the passwords you can create to the common subset of pdfdocenc and latin1 to deal with broken older software? | 13:00.22 |
| Robin_Watts | no, I suspect that when I try pasting in the euro sign it tries to paste in a unicode thing, and that gets rejected. | 13:02.28 |
| | When I paste in non-break-space, that works. | 13:02.44 |
| tor8 | so just being dumb eh? :) | 13:03.22 |
| Robin_Watts | Interesting. | 13:03.36 |
| | When I change the password encoding from anything-other-than-9 to anything-other-than-nine, the contents of the password field are kept. | 13:04.07 |
| | When I change it from anything-other-than-nine to nine, or vice versa, it blanks the password field. | 13:04.26 |
| | I bet anything-other-than-9 is taking 8 bit values, and 9 is taking unicode values. | 13:04.50 |
| | New Tigers.zip online - using command line infozip this time. | 13:07.34 |
| tor8 | that was my understanding before reading the extensionlevel 3 spec. passwords were always 8-bit values in whatever godforsaken codepage the user was using at the moment. | 13:07.36 |
| | Robin_Watts: the password actually used for the A0 files is " ", 5 regular spaces | 13:11.11 |
| Robin_Watts | Damn. | 13:11.29 |
| | OK. Let me try and find a util I used to have to enter chars by Alt + hex numbers. | 13:11.51 |
| | Looks like I need to reboot to try this. While I'm at it, I'm going to put the new HD into the case, so I'll be a few minutes. | 13:21.19 |
| | ok, new Tigers.zip online, where hopefully the passwords for the A0 ones really are what we want. | 13:55.12 |
| paulgardiner | robin_watts, tor8: annotation-access commit on paulg/master. One slight pain is that interactive apps now have to fall back to calling fz_run_page if idoc is NULL. | 14:55.13 |
| Robin_Watts | mvrhel_laptop: Morning. | 15:15.38 |
| | mvrhel_laptop: Dunno if bug 693668 is relevant/interesting/useful. | 15:16.11 |
| henrys | mvrhel_laptop: up early? | 15:21.24 |
| Robin_Watts | tor8: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=4c45666cb302a7eab95d362989c0e7e087d8383f | 15:45.12 |
| | paulgardiner: Looking now. | 15:45.17 |
| | hmm. | 15:46.53 |
| | I wonder if we should keep fz_run_page_contents as a doc thing, whos internal implementation tries to get an idoc from the doc, and if it can, calls the appropriate run_page_contents call, and if not just does fz_run_page. | 15:47.43 |
| | That would a) avoid the if/run_page/run_page_contents everywhere, and b) prevent everyone who has already written code that calls fz_run_page_contents having to track down a change which will only show up as a compiler warnings. | 15:48.53 |
| paulgardiner | I was wondering about making the current version of fz_run_page_contents special case idoc == NULL and call fz_run_page, but then it would need to be passed both doc and idoc | 15:50.14 |
| Robin_Watts | Getting idoc from doc is fast, right ? | 15:50.37 |
| paulgardiner | Yes | 15:50.44 |
| Robin_Watts | So I'd vote for having it get idoc from doc, and just passing the one. | 15:51.07 |
| | We could make ALL the functions take a doc, and get the idoc from it. | 15:51.30 |
| | but that starts to beg the question of why we'd have an externally visible fz_interactive at all. | 15:51.47 |
| paulgardiner | Yeah maybe. Means the interface loses its purity. | 15:51.59 |
| Robin_Watts | We'd still have the interface internally. | 15:52.10 |
| | And we wouldn't be bloating the fz_document_s. | 15:52.21 |
| | We need tor8 to weigh in here. | 15:53.18 |
| paulgardiner | Oh yes. I was thinking we'd need a virtual function, but no we wouldn't | 15:53.21 |
| Robin_Watts | We'd need an fz_is_interactive I guess. | 15:53.50 |
| | which would be "return fz_get_interactive(doc) != NULL;" | 15:54.12 |
| henrys | kens:you can start with the xps printer whenever you want. I'll be making improvements to the writer but I don't think that will effect your work. | 15:54.26 |
| paulgardiner | We already have fz_interact which takes a doc to an idco | 15:54.31 |
| kens | henrys : I'm busy with colour right now, I'll talk about it with you at the meeting ? | 15:54.53 |
| henrys | kens:sounds good | 15:55.06 |
| Robin_Watts | Right, I meant fz_interact when I said fz_get_interactive. | 15:55.11 |
| paulgardiner | So we just do idoc = fz_interact(doc); if (idoc) run_contents; else run; | 15:55.19 |
| kens | henrys I need to try some API calls and see what they do ;-) | 15:55.28 |
| Robin_Watts | paulgardiner: Sounds good to me. | 15:55.44 |
| tor8 | paulgardiner, Robin_Watts: umm. no. I don't like this at all. what does interactive have to do with page contents? | 15:59.28 |
| | Robin_Watts: alignment patch LGTM | 16:00.15 |
| paulgardiner | It's only when doing interactive stuff that you want to draw the main contents separately from the annotations | 16:00.21 |
| | The idea was fz_run_page takes a doc and draws the lot. fz_run_page_contents and fz_run_annot take an idoc | 16:01.30 |
| tor8 | right. I'll need to mull it over a bit. | 16:02.16 |
| | paulgardiner: do we really need the separate draw calls? I'm hazy about the details for why we need it. | 16:03.10 |
| | oops, gotta run! be back later. | 16:03.16 |
| mvrhel_laptop | Robin_Watts strange, I have not had any problems doing the build for the ARM config | 16:03.47 |
| Robin_Watts | we build for ARM for android without any problems. | 16:04.06 |
| paulgardiner | tor8: to allow for rendering only the anotation if only that has changed. | 16:04.26 |
| ray_laptop | morning, all | 16:45.07 |
| Robin_Watts | Morning ray_laptop | 16:50.51 |
| ray_laptop | Robin_Watts: The fix I put in for clist_copy_planes was flawed (commit 23796278). I forgot to multiply times the height (base/gxclrect.c line 913). Do you agree that it should be: | 16:52.25 |
| | if ((cdev->cend - cdev->cnext) < 0x100 + (maxheight * bytes_row * cdev->color_info.num_components)) | 16:52.27 |
| Robin_Watts | looking | 16:52.57 |
| ray_laptop | Robin_Watts: thanks. | 16:53.40 |
| Robin_Watts | This would be line 903 ? | 16:54.34 |
| ray_laptop | Robin_Watts: line 916 in my sources (master) | 16:55.29 |
| Robin_Watts | git diff 23796278~1..23796278 | 16:55.51 |
| | In that, it's in the chunk starting at line 903. | 16:56.09 |
| | maxheight = (cbuf_size - 0x100) / bytes_row / cdev->color_info.num_components | 16:56.38 |
| | So that's supposed to be the number of lines that will fit in the buffer, right ? | 16:56.49 |
| ray_laptop | Robin_Watts: right. line 916 in current source | 16:57.15 |
| | Robin_Watts: what I was trying to accomplish was to make sure we didn't flush the BufferSpace (cmd_write_buffer) in the middle of the planes of data | 16:58.13 |
| Robin_Watts | So it works out as: if ((cdev->cend - cdev->cnext) < cbuf_size), right ? | 16:58.38 |
| ray_laptop | because the cmd_write_buffer sticks an 'end_run' op into the stream | 16:58.41 |
| Robin_Watts | Wouldn't it be better to calculate max_height based on cdev->cend - cdev->cnext ? | 16:59.28 |
| ray_laptop | Robin_Watts: not really because we don't really want to split if we don't have to. The BufferSpace is LARGE (2Mb default) and sometimes larger | 17:00.29 |
| Robin_Watts | So you'd rather flush the buffer, than split it ? | 17:01.13 |
| | If you DO go with your change, you should add another change I think. | 17:01.48 |
| ray_laptop | Robin_Watts: so checking and calling cmd_write_buffer is OK | 17:01.50 |
| | Robin_Watts: Well there is another change I found was needed in some cases. Line 1016 (in master) of base/gxclrast.c | 17:02.41 |
| Robin_Watts | With your code, you're essentially saying "only ever put one of these things at the start of a clist buffer". | 17:02.43 |
| ray_laptop | Robin_Watts: No, that's NOT what I'm saying | 17:03.03 |
| Robin_Watts | What is cbuf_size ? | 17:03.12 |
| ray_laptop | Robin_Watts: oops. that should be plane_height (not maxheight). | 17:04.23 |
| Robin_Watts | Ah! | 17:04.34 |
| ray_laptop | Robin_Watts: that's why I wanted another pair of eyes. | 17:04.43 |
| Robin_Watts | I still don't think that's right. | 17:04.51 |
| ray_laptop | Robin_Watts: but maxheight is still OK (it is 4096, the size of the buffer used during clist reading) | 17:05.27 |
| Robin_Watts | Cos we aren't sending plane_height lines at a time. | 17:05.32 |
| ray_laptop | where as the BufferSpace is HUGE (relatively) | 17:05.49 |
| Robin_Watts | We are sending min(re.height,maxheight) lines at a time. | 17:06.14 |
| | Hence I think you should be using min(re.height,maxheight) in that calculation. | 17:06.33 |
| | You calculate maxheight to be "the maximum number of lines we can send at a time using complete buffers", yes? | 17:09.09 |
| | Then I think you should do: if (maxheight > re.height) maxheight = re.height; | 17:10.47 |
| | Then you can do your test: if ((cdev->cend - cdev->cnext) < 0x100 + (bytes_row * num_comps * maxheight)) | 17:11.24 |
| ray_laptop | Robin_Watts: now you are getting me confused. maxheight _is_ the most number of lines we can send at a time that fit in a 'cbuf' (the reading buffer) | 17:13.17 |
| Robin_Watts | Is that not what I said? | 17:13.42 |
| ray_laptop | Robin_Watts: yes, I was confirming that I agree with that part of what you said | 17:14.03 |
| Robin_Watts | Right. | 17:14.07 |
| | Now, we don't need to send maxheight lines, we only need to send re.height lines. | 17:14.37 |
| ray_laptop | Robin_Watts: yes. | 17:15.01 |
| Robin_Watts | Hence the data block that we will send needs to be at least bytes_row * re.height * num_comps in size. | 17:15.33 |
| | UNLESS re.height > maxheight. | 17:15.42 |
| | In which case we need to split the send into smaller chunks. | 17:15.57 |
| ray_laptop | so if I move the test until after re.height has been clamped to maxheight, then I can use re.height | 17:16.03 |
| Robin_Watts | hence we want to send: bytes_row * min(re.height,maxheight) * num_comps | 17:16.25 |
| | I think this code is missing a while. | 17:16.36 |
| | The comment at line 910 smells a bit like "Robin wrote this, and didn't know how to split this into several chunks" | 17:17.16 |
| ray_laptop | Robin_Watts: I don't see where it ever does the rest of the plane_height rows if it clamps re.height | 17:17.18 |
| Robin_Watts | ray_laptop: Indeed. I think the code doesn't cope with that :( | 17:17.35 |
| ray_laptop | Robin_Watts: OK. I will fix that (and see if I can spot it ever doing the wrong thing) | 17:18.06 |
| Robin_Watts | What we should do is to calculate maxheight (as you are doing). Then clamp maxheight to re.height. | 17:18.54 |
| ray_laptop | Robin_Watts: and I agree the check can go later, after re.height is clamped to maxheight and then use maxheight | 17:19.10 |
| Robin_Watts | Then go into a while... | 17:19.13 |
| ray_laptop | I mean re.height | 17:19.16 |
| Robin_Watts | and do your check/flush | 17:19.26 |
| | actually, let me start again. | 17:20.02 |
| | Calculate maxheight as you are doing. | 17:20.03 |
| | Then go into a while | 17:20.09 |
| ray_laptop | Robin_Watts: the conditional cmd_write_buffer _has_ to be in the loop that does the re.height rows, I agree | 17:20.21 |
| Robin_Watts | if (maxheight > re.height) maxheight = re.height; | 17:20.22 |
| | check/flush | 17:20.27 |
| | send maxheight lines. | 17:20.33 |
| | re.height -= maxheight; | 17:20.45 |
| | end while. | 17:20.47 |
| kens | Right, I have to go, goodnight all | 17:20.48 |
| ray_laptop | bye, kens | 17:20.57 |
| Robin_Watts | Night kens. | 17:20.58 |
| ray_laptop | Robin_Watts: we also have to adjust re.y in the loop, but I agree | 17:21.49 |
| Robin_Watts | ray_laptop: Right, yes. | 17:22.00 |
| ray_laptop | Robin_Watts: now can I ask you about base/gxclrast.c line 1016 ? | 17:22.34 |
| Robin_Watts | ok. | 17:22.51 |
| | That's "{" for me. | 17:23.29 |
| ray_laptop | Robin_Watts: this looks like it only does the top_up_cbuf check when pln > 0 | 17:23.44 |
| Robin_Watts | yes. | 17:24.03 |
| | It assumes that we've got space already on the first entry. | 17:24.16 |
| | (in particular, it must NOT do the compression = *cdp++ on the first entry) | 17:24.34 |
| ray_laptop | Robin_Watts: but if the copy_planes op was near the end of the cbuf, the data for it could go past the end of the cbuf | 17:24.37 |
| Robin_Watts | ray_laptop: and top_up_cbuf doesn't know how to cope with that? | 17:25.19 |
| ray_laptop | Robin_Watts: top_up_cbuf makes sure that the next byte to be read is at the start of the cbuf, and adjusts cbp to the start of the cbuf, filling in whatever it needs to in the cbuf | 17:26.28 |
| | but when pln==0 we skip the check | 17:27.04 |
| Robin_Watts | Right | 17:27.12 |
| ray_laptop | Robin_Watts: in fact, I tripped over one of these | 17:27.30 |
| Robin_Watts | This code is basically a copy_mono's code with a hack in it to run on a loop for each plane. | 17:27.54 |
| | And copy_mono has always coped with being called without such a topup call. | 17:28.13 |
| ray_laptop | I agree that the compression = *cbp++ should be when pln == 0, just not the check to see if top_up_cbuf is needed | 17:28.28 |
| Robin_Watts | ray_laptop: I can't see the harm in moving that check out of the if, certainly. | 17:29.09 |
| | Just, if it *needs* to be out of the if (i.e. if it needs to be called in the pln == 0 case) then copy_mono would be broken too. | 17:29.36 |
| ray_laptop | Robin_Watts: right -- I don't see how this could have worked | 17:31.00 |
| Robin_Watts | I'll bow to your greater understanding of this code. | 17:31.27 |
| ray_laptop | cmd_read_short_bits even has a comment that it may truncate reading at the end of the buffer | 17:33.29 |
| mvrhel_laptop | off to take a car in for repair. bbiab | 17:34.11 |
| ray_laptop | oh, wait. I mis-read cmd_read_data. It truncates reading from the buffer, but THEN it copies data directly from the stream to complete the read. So the code was OK. | 17:34.48 |
| | and we only need to check for the need to top_up_cbuf AFTER having read a plane, which may be bigger than a cbuf. | 17:35.43 |
| | Robin_Watts: so planes don't really have to fit in the cbuf_size at all, right ? | 17:37.11 |
| | Robin_Watts: as long as we don't end up doing cmd_write_buffer between the planes. | 17:38.06 |
| Robin_Watts | ray_laptop: Oh, urm... right. | 17:38.41 |
| | Assuming the clist is happy to have commands split between buffers with no headers on, sure. | 17:39.16 |
| | (i.e. you can't assume anywhere that every cbuf starts with a command header) | 17:39.38 |
| | So the topup being in the if was correct? | 17:40.31 |
| | and copy_mono was therefore safe? | 17:40.38 |
| ray_laptop | Robin_Watts: yes the top up being in the if was correct | 17:40.48 |
| Robin_Watts | right, that fits with my (appalling) memory. phew. | 17:41.05 |
| ray_laptop | Robin_Watts: my memory misdirects me sometimes too (at least in this mess). | 17:41.38 |
| | Robin_Watts: now I have to look at the writing to make sure it's OK, but I think it _has_ to be since we write VERY large chunks of data to the clist (eg. ICC profiles) | 17:42.51 |
| Robin_Watts | Right. But what we agreed before is now not right, yes? | 17:43.15 |
| ray_laptop | Robin_Watts: well, I don't think clamping re.height to what will fit in a cbuf is needed | 17:44.38 |
| Robin_Watts | cos we don't need to split so that height *(n planes of data) fits into a cbuf, but rather so that height * lines of data fits into a cbuf, planes times. | 17:44.38 |
| | Right. We still need the new while, with a new "check and flush if required" test in it. | 17:45.33 |
| ray_laptop | Robin_Watts: even a single plane can be larger than cbuf_size | 17:45.42 |
| Robin_Watts | but the conditions are a bit different. | 17:45.47 |
| | Can it? | 17:45.50 |
| ray_laptop | Robin_Watts: as long as all of the planes can be written without needing to do cmd_write_buffer | 17:46.24 |
| malc_ | tor8: any luck with scissoring problem? | 17:46.37 |
| Robin_Watts | ray_laptop: I don't follow that, but I trust you. | 17:47.19 |
| | malc_: I don't think either tor8 or I have had a chance to look at that yet. | 17:47.41 |
| | but I will endeavour too, soon, unless he's beaten me too it. | 17:48.07 |
| | to it. | 17:48.16 |
| ray_laptop | Robin_Watts: Well, when reading, we read the opcode and position and width and height (or the compression byte), then can read ANY amount of plane data | 17:48.46 |
| malc_ | Robin_Watts: oh, okay, looking forward to it | 17:49.19 |
| Robin_Watts | ray_laptop: When we read from the clist, we need the data to be held in memory in a format where we can call copy_planes | 17:49.44 |
| | That means that ultimately it needs to be assembled in a big block. | 17:49.59 |
| ray_laptop | Robin_Watts: so as long as the first byte of the plane data is in the cbuf, and as long as we top_up_cbuf AFTER reading a plane, we are OK since cmd_read_data just fills in from the stream (file) without buffering | 17:50.07 |
| Robin_Watts | I can't remember where that big buffer is. | 17:50.20 |
| ray_laptop | Robin_Watts: OK. That's where the restriction actuall comes in. We read the planes into a buffer called 'data_bits' and it just so happens that data_bits_size is #defined to be cbuf_size | 17:53.21 |
| Robin_Watts | ray_laptop: Right. | 17:53.34 |
| | So if we go back to restricting it at the writing side, in the way we agreed earlier, we'll be golden. | 17:54.00 |
| ray_laptop | Robin_Watts: but that #define is buried in gxclrast.c whereas cbuf_size is in the gxcldev.h header file | 17:54.20 |
| | Robin_Watts: so all of the planes _do_ have to fit in data_size (which is cbuf_size) | 17:55.04 |
| Robin_Watts | Sorry I should have commented all this better. (or I should have understood all this clearly enough to be able to comment it, and then commented it :) ) | 17:55.05 |
| | ray_laptop: Yes. | 17:55.10 |
| ray_laptop | Robin_Watts: I'll move data_size into the header file and then change the writer to use data_size (might make it somewhat less confusing) and explain that the reader holds the data for the copy_planes call in a buffer of data_size bytes, so we need to split it. | 17:56.29 |
| Robin_Watts | Sounds great. | 17:56.42 |
| ray_laptop | Robin_Watts: then I'll fix the splitting code to not just do the first part | 17:56.50 |
| | and, of course, make sure an do the check for cmd_write_buffer for each part of the split | 17:57.34 |
| | Robin_Watts: thanks again for you help sounding this out. | 17:58.26 |
| Robin_Watts | no worries. sorry the code was written badly to start with. | 17:58.38 |
| ray_laptop | Robin_Watts: that's not your fault -- the clist code has just had too much stuff tacked on, by too many people. Peter, John DeRosiers, me, Igor, me, Dan, mvrhel, you, me. And probably others | 18:02.17 |
| | oh, that's right. Stefan, too | 18:02.54 |
| | where do the git aliases live (how do I add one) | 18:20.34 |
| chrisl | ray_laptop: git config --global alias.<alias name> "<command>" | 18:26.10 |
| | So you'd do something like: git config --global alias.update "pull --rebase" | 18:26.47 |
| ray_laptop | chrisl: thanks. I tried git alias and that didn't help me :-/ | 18:32.32 |
| chrisl | ray_laptop: I started keeping a "git-hints.txt" a couple of weeks ago, for things I need to do regularly, but not often enough to memorize them | 18:34.23 |
| ray_laptop | chrisl: good idea | 18:34.34 |
| | oh, good. it will let you alias with the same name as an existing command. (I wanted blame to always use -w ) | 18:35.54 |
| | so that Tor doesn't get blamed for everything :-) | 18:36.15 |
| chrisl | Ugh, three tries to type the clusterpush command - I think I better finish for the night! | 18:39.12 |
| mvrhel_laptop | bbiaw | 19:42.32 |
| | Forward 1 day (to 2013/03/05)>>> | |
IRC Logs