| RAOF | racarr: Hey, I'm about to do a whole lot of commenting on introduce-bufferstream. | 00:51 |
|---|---|---|
| RAOF | racarr: Would you prefer an IRC discussion? :) | 00:51 |
| racarr | RAOF: Yeah IRC sounds better :) | 00:53 |
| RAOF | Ok. | 00:56 |
| RAOF | racarr: So, first off - why create_surfaceless_buffer_stream, rather than just create_buffer_stream? | 00:57 |
| racarr | RAOF: I guess because I'm thinking of BufferStream as analagous to an interafce implemented by | 00:58 |
| racarr | Surface and Screencast | 00:58 |
| racarr | so create_buffer_stream is | 00:58 |
| racarr | mildly incorrect? | 00:59 |
| RAOF | That brings up point 2 - why does MirScreencast still exist? :) | 01:00 |
| RAOF | So, my feeling is that mir_connection_create_surfaceless_buffer_stream is mildly incorrect; among other things, a screencast is *also* a surfaceless bufferstream. | 01:01 |
| racarr | hmm | 01:02 |
| racarr | I guess the Screencast C'Tor could just return BufferStream | 01:02 |
| racarr | except for ABI break | 01:02 |
| RAOF | Yeah, that's what I was thinking. | 01:02 |
| racarr | which this otherwise avoids iirc | 01:02 |
| RAOF | Yeah, this is true. | 01:03 |
| racarr | I guess, right so the other thing is | 01:03 |
| racarr | then you would need mir_buffer_stream_release | 01:03 |
| racarr | and have to document the behavior when called on a buffer stream obtained | 01:03 |
| racarr | from a surface | 01:03 |
| RAOF | Yeah. Which is my point 3 :) | 01:03 |
| RAOF | My preference would be for *all* buffer streams to need to be released. | 01:04 |
| racarr | oO? | 01:04 |
| racarr | What is the use case for releasing a surface but not the buffer stream? | 01:04 |
| RAOF | Consistency in API, mostly :) | 01:05 |
| RAOF | But it's also vaguely sensible, if you just care about output. | 01:06 |
| racarr | Hmm I dont know it seems to require a lot more error states, e.g. what happens if you use surface in when buffer stream is released, etc... | 01:06 |
| racarr | ? | 01:06 |
| RAOF | Neither the surface nor the buffer stream are destroyed until both are. | 01:06 |
| racarr | What do you mean by its vaguely sensible | 01:07 |
| racarr | if you just care about output? | 01:07 |
| RAOF | Well, if you're doing something like a video output window you could happily release the surface and just swap the buffer stream. | 01:10 |
| RAOF | But I agree that's not particularly compelling, which is why I said vaguely sensible :) | 01:10 |
| racarr | I guess I'd be more inclined to have mir_buffer_stream_release be an error | 01:10 |
| racarr | if the buffer stream is owned by a surface | 01:11 |
| racarr | or something...not sure of the exact language | 01:11 |
| RAOF | Well, *that* could actually be a sensible usage. | 01:11 |
| racarr | ? | 01:11 |
| RAOF | If you're just doing say a static splash screen - create the surface, get the buffer stream, do your one and only swap, then release the buffer stream and now Mir can free some memory :) | 01:11 |
| RAOF | But eh. | 01:12 |
| RAOF | Mostly I want it for API consistency, and for when we want to associate multiple buffer streams with a single surface. | 01:12 |
| racarr | I am not sure I would quite see it | 01:13 |
| racarr | anything that requires every currently existing call call to mir_surface_release | 01:13 |
| racarr | literally without fail | 01:13 |
| racarr | to be prefixed with a call to mir_buffer_stream_release | 01:13 |
| racarr | has to be incorrect right? | 01:13 |
| racarr | anyone looking would say the surface should own the buffer stream and take care of releasing it.... | 01:14 |
| RAOF | Well, I thought the same thing for MirSurfaceSpec; mir_surface_create() should eat the SurfaceSpec passed in. | 01:15 |
| racarr | ugh | 01:15 |
| racarr | lol | 01:15 |
| RAOF | But I don't think it's so unreasonable now :) | 01:15 |
| racarr | This is a little different though because the surface creates the buffer stream... | 01:15 |
| racarr | and I do think its unreasonable lol | 01:16 |
| racarr | floating references provide a better solution | 01:16 |
| racarr | but yes I guess that landed | 01:16 |
| duflu | racarr: a buffer stream allocator for clients might be what we need to make nested clients bypass/overlayable too | 01:17 |
| duflu | ... as the nested server can then allocate buffers from the system compositor for nested clients | 01:17 |
| RAOF | Also, you still have the problem of error states multiplying - you still have to specify what happens when you use buffer stream you got from a released surface. | 01:17 |
| racarr | lol | 01:17 |
| racarr | nah, it becomes invalid | 01:17 |
| racarr | I guess there should be | 01:17 |
| racarr | mir_buffer_stream_is_valid | 01:17 |
| RAOF | Yeah, that's necessary. | 01:18 |
| RAOF | duflu: Oooh, exactly right! | 01:18 |
| RAOF | duflu: That's ‘multiple buffer streams per surface’. | 01:18 |
| duflu | RAOF: Nah, just the nested server needs a way of allocating system-compositor buffers for nested clients | 01:18 |
| RAOF | Also, hello from Perth. | 01:18 |
| duflu | RAOF: Oh good morning | 01:19 |
| racarr | Hmm ok I am convinced mir screencast ctor should return MirBufferStream | 01:19 |
| racarr | I am convinced...surfaceless_buffer_stream could have a better name | 01:19 |
| duflu | racarr: minus "surfaceless" because that's kind of implied | 01:19 |
| duflu | Bugger. We're too efficient and landed the branch I was about to fix | 01:20 |
| racarr | I mean not necessarily some buffer streams are associated with surfaces | 01:20 |
| duflu | Another branch then | 01:20 |
| racarr | some aren't | 01:20 |
| racarr | but RAOF points out that screencast isnt associated with a surface | 01:20 |
| racarr | but isnt what I call a SurfacelessBufferSTream | 01:20 |
| duflu | racarr: Yeah there's a reason why it's called a stream in video encoding/decoding | 01:21 |
| racarr | RAOF: The part that I can't wrap my head around is why you should have to release a buffer stream seperately from the surface.... | 01:22 |
| racarr | duflu: ? | 01:22 |
| duflu | racarr: The reason is just that nobody has thought of a better abstraction/noun than "stream" | 01:22 |
| racarr | oh lol | 01:22 |
| duflu | And maybe there isn't one | 01:22 |
| RAOF | racarr: You don't have to, but I feel it's a bit cleaner and consistent. | 01:22 |
| racarr | RAOF: Hmm | 01:23 |
| racarr | I guess I will mull over that one my thought is that | 01:23 |
| racarr | the consistency is "all streams which you allocat ehave to be released" and in this case it | 01:23 |
| racarr | s said that the surface allocates the stream | 01:23 |
| racarr | likewise I would say that in the case of attaching multiple buffer streams to surfaces, etc... | 01:23 |
| racarr | I guess I would expect the surface to take ownership of the stream | 01:23 |
| RAOF | I'm happy with that, but I'm not sure it's consistent with our current API. | 01:26 |
| racarr | im not sure either lol | 01:26 |
| racarr | wrt surface spec its different because you allocate the spec... | 01:26 |
| racarr | hmmm | 01:26 |
| RAOF | But then with your multiple-buffer-streams case you *also* allocate the buffer stream :) | 01:27 |
| racarr | and are we sure there really is no MirScreencast | 01:27 |
| racarr | I can almost imagine things like | 01:27 |
| racarr | mir_screencast_set_capture_rate, or mir_screencast_set_scale | 01:27 |
| RAOF | Maybe? | 01:28 |
| racarr | What if we add MObject and then MirScreencast can MIR_IMPLEMENT_INTER.... | 01:28 |
| racarr | err.... | 01:28 |
| RAOF | :) | 01:28 |
| RAOF | All nontrivial C programs are guaranteed to implement a half-arsed object system? :) | 01:29 |
| racarr | mm | 01:29 |
| racarr | it does kind of seem like set_rate could be a thing...-.- | 01:33 |
| RAOF | That would seem to be mir_buffer_stream_set_swap_interval? | 01:33 |
| racarr | hmm probably | 01:34 |
| racarr | Seems Screencast can probably go | 01:35 |
| racarr | RAOF: Ok so my plan is to change the surfaceless_buffer_stream names, drop MirScreencast, add buffer_stream_is_valid | 01:36 |
| racarr | do something about buffer_stream_release | 01:36 |
| racarr | (took notes) | 01:36 |
| RAOF | Hm. | 01:38 |
| RAOF | Does MirCursorConfiguration want to own the buffer_stream? | 01:40 |
| racarr | hmm | 01:41 |
| racarr | I dont think so at least from the client perspective | 01:42 |
| racarr | and thats consistent with surface spec | 01:42 |
| racarr | on the server side though, if the buffer stream was used as the | 01:42 |
| racarr | active cursor request from a surface | 01:42 |
| racarr | even if you release the buffer stream | 01:42 |
| racarr | the surface will keep around shared_ptr buffer stream and the last buffer will persist until | 01:42 |
| racarr | you make a new cursor request for the surface | 01:42 |
| racarr | which I thought was good semantics | 01:43 |
| racarr | it would be more obvious with unref vs release | 01:43 |
| RAOF | Maybe we want unref? | 01:44 |
| RAOF | I agree that's good semantics. | 01:44 |
| racarr | I kind of think we want unref, because...well I dunno, that semantic might not be entirely obvious to me as I see _release | 01:46 |
| racarr | right. I mean I guess its explicitly non obvious | 01:46 |
| racarr | because release typically means release | 01:46 |
| racarr | however if I saw unref | 01:46 |
| racarr | I would assume | 01:46 |
| racarr | the cursor configuration and or surface took references to the buffer stream | 01:47 |
| RAOF | Right. | 01:47 |
| RAOF | (Relatedly, Ryan is correct in saying that we need a destroy notify on our delegates) | 01:47 |
| racarr | indeed. | 01:48 |
| racarr | I'm making event ref counted in another mp... | 01:49 |
| RAOF | Which would happily remove the need for all our _release_sync()s. | 01:49 |
| racarr | maybe connection and surface should be ref counted too... | 01:49 |
| racarr | RAOF: ? | 01:49 |
| racarr | Oh I see | 01:49 |
| racarr | wow this reminds me of an ancient problem that _create_sync is unsafe because oyu dont have time to set delegates before | 01:49 |
| racarr | events can be received | 01:49 |
| racarr | and that delegates need to be in surface spec | 01:50 |
| racarr | <tangent> | 01:50 |
| racarr | well its unsafe in that you could miss events | 01:50 |
| racarr | and will miss your first focus event | 01:50 |
| racarr | Total tangent lol | 01:50 |
| racarr | ok hopefully I should get to all this tomorrow assuming I dont wake up to some sort of perfect storm of merge conflicts | 01:52 |
| racarr | im kind of assuming I will wake up to a perfect storm of merge conflicts but who knows | 01:52 |
| RAOF | Heh. | 01:53 |
| racarr | Ok. Off to go play some piano before dinnnnner | 01:58 |
| racarr | RAOF: Thanks :) | 01:58 |
| duflu_ | RAOF: Can you check this out some time soon? ... https://code.launchpad.net/~vanvugt/mir/revert-r2165/+merge/245063 | 02:34 |
| RAOF | Sure. I'm doing a big review sweep anyway. :) | 02:36 |
| === duflu_ is now known as duflu | ||
| racarr | whee merging sha1sums causes cascading jenkins failures again! | 03:13 |
| racarr | duflu: I wonder how evil MirEvent* mir_event_ref(MirEvent const*) is :p | 04:13 |
| racarr | I think its ok if you believe in logical constness | 04:13 |
| racarr | you mark the ref count mutable the compiler is happy | 04:13 |
| racarr | all calls mir_event_foo* return the same value | 04:14 |
| racarr | after mir_event_ref that they did before mir_event_ref | 04:14 |
| duflu | racarr: I think I abstained on that? | 04:14 |
| racarr | so its 'logically const' right | 04:14 |
| * duflu checks | 04:14 | |
| RAOF | Uuuh, you mean MirEvent const* mir_event_ref(MirEvent const*), right? | 04:14 |
| racarr | duflu: Mm :) I am just saying maybe there wont be an ABI break | 04:14 |
| RAOF | I'd be fine with that; you're not changing the contents of the MirEvent. | 04:14 |
| racarr | RAOF: Even better! | 04:14 |
| racarr | lol | 04:14 |
| racarr | hmm | 04:15 |
| racarr | no | 04:15 |
| duflu | racarr: Mutable or not, it would be poor C design to go changing things insider a const object | 04:15 |
| racarr | RAOF: But then mir_event_unref would have to accept a const | 04:15 |
| racarr | MirEvent | 04:15 |
| racarr | which...is less | 04:15 |
| racarr | more questionable | 04:15 |
| racarr | I dont think youc an say its logically const | 04:16 |
| racarr | if you destroy the object | 04:16 |
| racarr | lol | 04:16 |
| RAOF | Why does MirEvent want to be refcounted, by the way? | 04:16 |
| duflu | racarr: Non-const always, but just work to make the return value optional later? | 04:16 |
| duflu | Then no ABI break | 04:16 |
| racarr | duflu: Unfortunately clients only ever get MirEvent const* | 04:16 |
| duflu | Although this is mircommon, which we break regularly. Possibly a non-issue | 04:16 |
| RAOF | I don't know. I think it *is* logically const if mir_event_unref() only ever destroys an object that's no longer referenced anywhere. | 04:16 |
| racarr | oh | 04:16 |
| racarr | thats true | 04:16 |
| racarr | haha | 04:17 |
| racarr | well | 04:17 |
| racarr | thats an argument at least! | 04:17 |
| racarr | hmm | 04:17 |
| racarr | effectively it wants to be refcounted because it wants to be copied | 04:17 |
| RAOF | Because the client code wants to preserve it outside the event callback? | 04:17 |
| duflu | racarr: This is kind of why I was saying ABI breaks are inherited though. e.g. some changes in mircommon would break the client ABI | 04:18 |
| racarr | RAOF: Basically, and because | 04:18 |
| racarr | the client library code isnt going to access it after that | 04:18 |
| racarr | I dont think you gain any sort of avoid synchronization benefit from a copy | 04:18 |
| racarr | and ref will be most efficient | 04:18 |
| racarr | RAOF: Then the double controversy is because I can't add ref_count to MirEvent yet mir_event_ref currently copies the event | 04:18 |
| racarr | :p | 04:18 |
| RAOF | Well, if you only ever use it inside the event callback then you can just pass the const*. | 04:19 |
| RAOF | What code wants to use the event outside the callback? | 04:19 |
| duflu | racarr: Surely while it's not opaque the user can just copy it? :) a = *b | 04:19 |
| racarr | RAOF: Qt queues it up as a task on the main loop | 04:19 |
| racarr | duflu: That's what qtubuntu does now | 04:19 |
| racarr | and im trying to | 04:19 |
| racarr | port it | 04:19 |
| racarr | :) | 04:19 |
| RAOF | Ah, fair enough. | 04:19 |
| RAOF | Then I'm for mir_event_ref and mir_event_unref, both working on const* | 04:20 |
| racarr | mm I think so too | 04:21 |
| racarr | will update in morning | 04:21 |
| RAOF | duflu: std::atomic_bool _still_ isn't equivalent to std::atomic<bool> :) | 05:20 |
| anpok | RAOF: hmm it cannot find the graphics-dummy platform | 05:34 |
| * RAOF looks. | 05:36 | |
| RAOF | Oh, on the mako? | 05:37 |
| anpok | hm there we install the packages and do not run from build location | 05:38 |
| anpok | RAOF: oh you meant it should =log on the client and server by default? | 05:39 |
| RAOF | anpok: Yes. | 05:39 |
| RAOF | anpok: But more; I don't think that it should be a report at all. It was just - when all you have is a ReportPattern, everything seems to be a report. | 05:40 |
| anpok | :) | 05:40 |
| RAOF | Because why would you _ever_ want to use SHARED_LIBRARY_PROBER_REPORT=lttng | 05:41 |
| RAOF | ? | 05:41 |
| anpok | RAOF: if all your reports report to lttng.. all your reports report to lttng | 05:41 |
| anpok | to be honest I only made that change, because I needed it for the input side | 05:44 |
| RAOF | Yeah. | 05:45 |
| RAOF | I think that short-term defaulting to log is fine. | 05:45 |
| RAOF | And then, longer term, we just need to drop the SharedLibraryProberReport interface entirely, move mir::logging into mircommon, and then have libraries_for_path log directly, rather than take a SharedLibraryProberReport. | 05:47 |
| RAOF | anpok: Oh, of course! | 05:51 |
| RAOF | anpok: You're missing a path separator in the as-installed probe. | 05:53 |
| RAOF | anpok: Would you like to fix it, or shall I? | 05:53 |
| RAOF | anpok: Specifically: | 05:54 |
| RAOF | + {library_path() + "/server-modules/", library_path() + "/server-platform/", std::string(MIR_SERVER_PLATFORM_PATH)}) | 05:54 |
| RAOF | should be | 05:54 |
| RAOF | + {library_path() + "/server-modules/", library_path() + "/server-platform/", std::string(MIR_SERVER_PLATFORM_PATH) + "/"}) | 05:54 |
| RAOF | Because MIR_SERVER_PLATFORM_PATH=/usr/lib/linux-armhf-gnueabi/server-platform | 05:55 |
| duflu | RAOF: Not equivalent, but more efficient and sufficient possibly | 05:58 |
| duflu | Although atomic anything is open to races. I think we're turning a blind eye to that most of the time | 05:59 |
| duflu | Hmm, the sensible thing to do would be to find ways to tie up loose ends by tomorrow | 06:02 |
| duflu | Not sure how to achieve that more than we already are :P | 06:02 |
| RAOF | duflu: Actually, now that I've looked a bit more, I take that back. | 06:03 |
| RAOF | std::atomic_bool is a typedef to std::atomic<bool>. | 06:03 |
| RAOF | So we're both wrong :) | 06:03 |
| duflu | RAOF: Regardless if you know enough about the arch it could also be typedef bool :) | 06:03 |
| duflu | sigactomic_t | 06:03 |
| duflu | sigactomic_t | 06:04 |
| duflu | or sig_atomic_t | 06:04 |
| duflu | one of them | 06:04 |
| RAOF | I don't think sig_atomic_t is guaranteed to be thread-atomic. | 06:04 |
| RAOF | And it can't be typedef bool, because there's memory barriers involved. | 06:05 |
| duflu | RAOF: It's a pretty safe bet if the type is within the word size | 06:05 |
| duflu | RAOF: Barriers?! | 06:05 |
| RAOF | Yes, barriers? | 06:06 |
| duflu | Oh I see. std::atomic is quite featured. | 06:06 |
| duflu | Too many features for some uses | 06:06 |
| duflu | RAOF: I guess not if is_lock_free() | 06:08 |
| RAOF | Barriers != locks. | 06:08 |
| RAOF | Barriers are “dear CPU+compiler, please actually read this memory” | 06:08 |
| duflu | RAOF: ?? | 06:08 |
| racarr | e.g. cache invalidation | 06:09 |
| RAOF | Compilers are free to reorder or entirely elide reads; likewise CPUs (and, indeed, multiple levels of the memory heirarchy) | 06:09 |
| duflu | Interesting. I always use spinlock as my lowest level primitives but seems like barriers might also be useful | 06:11 |
| RAOF | duflu: Feel like a quick and easy top-approve? | 06:12 |
| RAOF | duflu: https://code.launchpad.net/~mir-team/mir/more-inconsistent-overrides/+merge/245068 | 06:12 |
| RAOF | racarr: :P | 06:14 |
| RAOF | duflu: http://en.wikipedia.org/wiki/Memory_barrier if you're interested. In particular, memory barriers are required to correctly implement any synchronisation primitive. :) | 06:15 |
| duflu | RAOF: Right. Back on the original topic. If you can avoid a template instantiating type then it's obviously preferable. Some compilers at least will extern the std::atomic_foo methods | 06:18 |
| duflu | Or extern the instantiation | 06:18 |
| RAOF | The instantiation is in libstdc++, though. | 06:18 |
| duflu | RAOF: Yes, in some implementations. What's important is that it might be typedef'd to something that's not an instantiation, hence more efficient | 06:19 |
| RAOF | I'm not entirely sure how that follows? | 06:24 |
| duflu | RAOF: Not mentioning '<>' in the code ensures you don't slow down the compiler to check for or create instantiations | 06:31 |
| duflu | Unfortunately not an advantage if the library typedefs to one | 06:31 |
| duflu | But other C++ implementations would benefit | 06:32 |
| duflu | I don't remember the end of any year ever being this busy | 06:48 |
| racarr | The year jenkins stole christmas | 06:48 |
| duflu | racarr: To be fair to Jenkins, it didn't slow us down. We had already landed everything we could. It just sucked up my time landing things | 06:49 |
| racarr | I dunno not to give anyone a hard time because I did the same thing to some branches but I assume introduce-buffer-stream etc wouldnt have gone a full week | 06:55 |
| racarr | without review | 06:55 |
| racarr | if they had a +1 from jenkins | 06:55 |
| racarr | or maybe europe just doesnt like me anymore because I sleep through standup and there are only two of you guys | 06:56 |
| racarr | who knows | 06:56 |
| duflu | racarr: I know how it feels. Safe to say everyone's waiting for things of theirs to lang | 07:00 |
| duflu | land | 07:00 |
| racarr | :) | 07:01 |
| duflu | And I'm now involved in more code reviews than I can feasibly keep up with. So staying out of others | 07:01 |
| racarr | Thats how I felt towards EOD today toooo | 07:01 |
| racarr | Ok daily show -> bed | 07:01 |
| duflu | racarr: As a matter of psychological trickery, developers tend toward reviewing smaller things first | 07:02 |
| duflu | Not that that's a "trick" but helpful to know | 07:02 |
| anpok | RAOF: uh! | 07:08 |
| === chihchun_afk is now known as chihchun | ||
| === chihchun is now known as chihchun_afk | ||
| RAOF | Boo. | 08:04 |
| RAOF | Fewer random hard shutdowns please laptop. | 08:05 |
| mlankhorst | RAOF: hey did you take a look at standalone xmir yet? :P | 08:07 |
| RAOF | mlankhorst: What did you want me to look at again? | 08:07 |
| RAOF | I've been off for the last 3 days, so may have lost some state :) | 08:07 |
| mlankhorst | no just in general if you have looked at it or not | 08:08 |
| RAOF | mlankhorst: Oh, no, I've not. | 08:28 |
| RAOF | mlankhorst: Is there anything you'd like me to do about it? | 08:29 |
| mlankhorst | not in particularly, just if you could look it over | 08:31 |
| === chihchun_afk is now known as chihchun | ||
| === jono is now known as Guest60113 | ||
| Mark__ | hello, anyone here? | 15:05 |
| === chihchun is now known as chihchun_afk | ||
| === alan_g is now known as alan_g|EOD | ||
| racarr | can I TA add-more-event-getters? | 22:17 |
| racarr | Happy to lose the dependency on my branches... | 22:17 |
| racarr | *removes MirScreencast from public API* | 22:34 |
| racarr | Chris pointed out last night MirBufferStream* mir_connection_create_screencast can be enough | 22:35 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!