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