mozilla :: #gfx

17 Apr 2017
13:09pulsebotCheck-in: https://hg.mozilla.org/projects/graphics/rev/f3878c1cd4d9 - sotaro - Bug 1356960 - Avoid to allocate WebRenderTextureHost for TextureHost of VideoBridgeParent and VRManagerParent r=jerry
13:53katspchang: are you around?
13:53pchangkats: yes
13:54katspchang: are you done with the patchset on bug 1345017? every time i review it have to go through the whole patchset to remember everything so i'd like to hold off on reviewing until you're done
13:54firebothttps://bugzil.la/1345017 ASSIGNED, howareyou322@gmail.com Add SampleAnimation support
13:55pchangkats: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9e1681b0be12ce119922289dec870f350a0a18a
13:56katspchang: ok i'll wait until you have that sorted out. post on the bug when you have a green try push and i'll do the review then
13:56pchangkats: I created another patch to fix the try failures, but right now I still got four reftest failures. How do you think if I create a follow up bug to fix the failure?
13:57katspchang: do you understand what's causing the failure?
13:57pchangkats: looks like the transform offset still has problem for specific use case. I already fixed most of them.
13:58katspchang: how hard is it to fix? i'd rather not introduce more reftest failures if possible
13:58katsor maybe if there's some patches on the bug that are ready to land and don't cause try failures we can split up the bug
13:59katsland the stuff that's green and put the rest in another bug?
14:00pchangkats: then I would just add a preference for omta with wr. And then I can work on the try failures
14:00katspchang: that sounds fine to me
14:01pchangkats: ok, I will update patches and notify you soon
14:01katspchang: thanks
14:01pchangkats: thanks for the reviewing
14:01katsnp
14:01pulsebotCheck-in: https://hg.mozilla.org/projects/graphics/pushloghtml?startID=481&endID=482 - 20 changesets - Merge m-c to graphics
14:16pchangjrmuizel_: ping
14:16jrmuizel_pchang: pong
14:16pchangjrmuizel_: are you the gfx channel admins?
14:17jrmuizel_pchang: I am
14:17pchangjrmuizel_: could you type " /invite standups" to invite the standups bot?
14:17katsright now it looks like there's no admin
14:17katsyou probably need to drop the _ in your nick
14:18pchangjrmuizel: I would like to use this bot to record tpe gfx daily status in https://www.standu.ps/
14:18jrmuizelpchang: I
14:18jrmuizelpchang: I've not had a lot of success inviting bots before
14:19jrmuizelpchang: you can try inviting the bot
14:20pchangstandups: I will land omta default off with wr in bug 1345017
14:20standupsOk, submitted #44842 for https://www.standu.ps/user/pchang/
14:20firebothttps://bugzil.la/1345017 ASSIGNED, howareyou322@gmail.com Add SampleAnimation support
14:20pchangjrmuizel: I just invite the bot. Thanks
14:20jrmuizelpchang: greaet
14:42pchangkats: I just uploaded the patch to disable omta and I will check the try result tomorrow
14:42pchangdisable omta with wr for now
14:43katspchang: ok, thanks. i'll keep an eye on the try result as well. if it looks green i'll probably just do the review
14:44pchangkats: thanks
14:46pulsebotCheck-in: https://hg.mozilla.org/projects/graphics/pushloghtml?startID=482&endID=483 - 39 changesets - Merge m-c to graphics
14:51katspchang: build failure on your try push
14:51pchangkats: checking
14:52katspchang: also while your'e at it, please call the pref gfx.webrender.omta.enabled / WebRenderOMTAEnabled()
14:53pchangkats: sure
14:53katsthx
15:02pchangkats: so I should use this https://github.com/rlhunt/cbindgen instead of wr-binding. Am I right?
15:03katspchang: yeah
15:03pchangkats: I see because I tried to generate new ffi with wr-binding and caused build failure
15:03katspchang: yeah sorry about that, we updated the tool used
15:08pchangkats: do you know how to get 'cbindgen'? before calling cbindgen -c wr gfx/webrender_bindings gfx/webrender_bindings/webrender_ffi_generated.h
15:08katspchang: did you build it locally, or did you install via 'cargo install'? if you built it locally it should be in your target/debug or target/release folder
15:09katsif you installed via 'cargo install' it should be in ~/.cargo/bin/cbindgen
15:11pchangkats: thanks, I just use cargo install . I think we probably need to update the usage from https://github.com/rlhunt/cbindgen
15:12katsGankro: bug 1355475 is the one currently tracking the next webrender update. once we fix the issue and land the update i clone the bug to a new one to track the next update
15:12firebothttps://bugzil.la/1355475 NEW, nobody@mozilla.org Future webrender update bug
15:13rhuntpchang: sorry about the churn
15:13rhuntpchang: what do you mean, update the usage?
15:14pchangrhunt: I guess we should call 'cbindgen -c wr gfx/webrender_bindings gfx/webrender_bindings/webrender_ffi_generated.h'
15:15pchangrhunt: a little different with 'cbindgen crate/ crate/bindings.h' in the github
15:15rhuntpchang: ah, yeah
15:15katspchang: cbindgen is becoming more generalized now, so the instructions are more generic. the more specific instructions are in webrender_ffi_generated.h
15:16pchangrhunt: so the -c wr is WR only, do we need to write down it somewhere?
15:16pchang'-c wr'
15:17katspchang: https://hg.mozilla.org/projects/graphics/file/tip/gfx/webrender_bindings/webrender_ffi_generated.h#l7
15:17pchangkats: good to know
15:18rhuntpchang: yes, so my plan has been to keep up to date instructions in the autogen file, including '-c wr'
15:18rhuntpchang: if it helps, we could put this in README.webrender
15:19rhuntpchang: if possible, this could also be automated in the build process
15:19pchangrhunt: that would be great
15:20rhuntpchang: putting instructions in README.webrender or automating the generation? :)
15:21pchangrhunt: I mean automating the generation ^^
15:21pchangrhunt: I'm fine we noted in webrender_ffi_generated.h header
15:21pchangrhunt: I will share to taipei folks
15:22Gankrokats: thanks! sounds like you expect to land this in the next few days?
15:22katsGankro: whenever the regression is fixed, and assuming there's no other regressions. probably this week, yes
15:23katsGankro: i believe monday and tuesday are holidays in australia so glenn probably won't look at the regression until wednesday anyway
15:23Gankrokats: the regression is minor enough that I could just build of your branch to test perf, eh?
15:23katsGankro: yup, you could do that
15:23rhuntpchang: yes, i agree, and thanks!
15:26rhuntkats: what are our options for using cbindgen at build time? would it have to be vendored into m-c?
15:26rhunthm, i wonder what rustbindgen does
15:26katsrhunt: if we want to run it unconditionally (i.e. on every build) then yes
15:27katsrhunt: if we want to have a special mozconfig option to run it, then it may not need to be
15:27katsi know that you can enable/disable rust bindgen in the stylo build
15:27katse.g. with ac_add_options --disable-stylo-build-bindgen
15:29rhuntkats: i don't think we need everyone to run it, so maybe not then
15:30rhuntkats: i would just not want to bloat m-c with dependencies like syn, or clap
15:30katsrhunt: syn is already in m-c
15:30katsas is clap
15:30rhuntreally? huh
15:30katsrhunt: it's in third_party/rust/
15:31rhuntkats: ah I see, do you know what depends on it?
15:32rhuntkats: as is serde_json which was the only other dependency I was considering adding
15:32katsrhunt: looks like bindgen :) http://searchfox.org/mozilla-central/source/toolkit/library/rust/Cargo.lock#64
15:33katsit's probably best to just vendor it
15:33rhuntkats: yeah that makes sense to me
15:33katswe can add it to the dev-dependencies of webrender_bindings which will make 'mach vendor rust' pick it up automatically
15:34katsalthough we probably want to run it from the moz.build file rather than from a build.rs file, because it needs to generate the header early
15:34katsi'm not sure offhand how to do that
15:34katsprobably in the same part of the build cycle that generates .h file from .ipdl files
15:35rhuntdoes cargo deps for binary crates work? i.e, cbindgen is an executable currently
15:36katsrhunt: it should
15:36katsrhunt: i imagine rust bindgen is also an executable
15:36katsspecially if it requires clap
15:37emiliokats: rhunt: It's _not_ an executable, but we depend on clap because rust doesn't allow you to specify bin-only or lib-only dependencies. We could get rid of that dependency for stylo if needed.
15:38katsah. regardless i don't see why cargo-vendor wouldn't work for binary crates
15:39emiliokats: right, thought I don't know how does that fit in Gecko's build process
15:41emiliokats: in the sense of: You need to compile an external binary, and run it from the build system. I don't think anybody does that now (everything ends up holding from gkrust IIRC). But I suppose it can be done.
15:41katshm yeah i don't think anybody does that now either
15:42katsemilio: but i think it should be doable
15:42rhuntkats, emilio: worst case, I can modify cbindgen to work as a lib
15:42katsemilio: i know we support building standalone rust programs from moz.build, e.g. https://github.com/staktrace/gecko-dev/commit/d9b23bc4f51beccc002d70aa159f830cf2ded75a#diff-483d61608cf0415cf961b4dd0563cf2b
15:42rhunti might want to do that regardless
15:48rhuntkats: I can take a look at this and let you know how it goes
15:50katsrhunt: sure
15:54dvanderMilan do you want help looking at those gpu process assertions?
16:12milandvander: sure, I may not be able to get to it right away and you'd understand if they're real or if the main thread assertions should go away when in the GPU process
16:49rhuntjrmuizel: did you say that wrench replay is broken?
17:14Gankrokats: ok so I've (blessedly) never had to deal with bugzilla -- how do I get whatever code your update issue is pointing to?
17:16katsGankro: so there are links to treeherder.mozilla.org which are pushes to the "try" tree. Click on the link in the last comment, which will give you the latest test push. The commits listed there are the mercurial changesets that went into the push
17:16katsGankro: are you using mercurial or git for mozilla-central?
17:16Gankrokats: mercurial atm
17:16katsGankro: ok that makes it easier
17:17katsyou can do hg pull -r bc74f0363c81 https://hg.mozilla.org/try to pull the commits
17:17katswhere the bc74f0363c81 commit is the topmost commit with an actual code change
17:18katsyou could pull f2b3d2c54830 if you wanted but that just has a commit message with try syntax and no code changes
17:19katsGankro: once you pull it it should show up as a head in your local repo with `hg heads .` and then you can check it out using `hg update bc74f0363c81` or whatever
17:20Gankrokats: should I do any prep step so I can easily revert to "master"?
17:21katsGankro: if you want you can set a bookmark on it. but you should already have 'central' mapped to it assuming you ran the bootstrap and have the usual version-control-tools extensions
17:21katsGankro: you can check by running 'hg heads .' first
17:22Gankroso "hg update central" is ~"git checkout master" ?
17:22katsGankro: yeah more or less
17:24Gankrokats: ok looks good, thanks for the quick tutorial!
17:24* Gankro writes a script for it
17:24katsGankro: np
17:24katsGankro: generally pulling from a try push is not something that happens very often
17:25Gankrokats: All the more reason to store the knowledge in a script :)
17:28GankroIn retrospect this is a ridiculously simple script but hey who wants to write the `try` URL
17:57Gankrobuilding firefox with my changes:
17:57Gankro 0:21.90 error: the lock file needs to be updated but --locked was passed to prevent this
17:57GankroAnyone know what this means/how to deal with it?
18:03rhuntGankro: maybe run ./mach vendor rust?
18:08katsGankro: yeah that means that you changed a Cargo.toml somewhere that changed either version numbers of in-tree libraries or changed the dependencies that got pulled, but the Cargo.lock for libgkrust and/or the vendored dependencies in third_party/rust weren't updated
18:08katsGankro: `mach vendor rust` in theory should do the necessary changes. sometimes it doesn't though, so let me know if you still have problems
18:08GankroInstalling cargo vendor, will know soon!
18:11GankroSeems to have worked \o/
18:12katscool
18:37rhuntkats: so I can see how we'd vendor cbindgen and could compile it as a standalone, but I'm not sure how we can run it during the build
18:38rhuntkats: have any ideas?
18:38katsrhunt: i'm sure it's doable but i'm not sure exactly how to do it either. maybe ask froydnj?
18:39rhuntkats: will do
19:53GankroWhat's the magic incantation to rebuild webrender_ffi_generated.h?
19:54rhuntGankro: https://hg.mozilla.org/projects/graphics/file/tip/gfx/webrender_bindings/webrender_ffi_generated.h#l7
19:54Gankrorhunt: ty
19:58Gankrorhunt: is it fine that I got like 50 warnings
19:59rhuntGankro: potentially, what are some of them?
19:59Gankro"WARN: can't find extern crate core"
19:59rhuntGankro: ah yeah that's fine
19:59Gankro"WARN: can't find T"
20:00Gankrocan't find uint32_t is a bit disconcerting
20:00Gankrorhunt: is this the only file I need to update if I changed the fields in some structs?
20:00rhuntyeah, those are all known issues
20:00rhuntthe extern crates are fine, because we don't need anything from them
20:01rhuntthe can't find T or uint32_t are a spurious warning from the generics code that will be fixed
20:02rhuntGankro: did you modify structs in bindings.rs?
20:02Gankrorhunt: no, stuff in webrender and webrender_traits only
20:03rhuntGankro: or I guess, if the structs you changed are used in gecko you might have to update their usage, but for the bindings that's it
20:03Gankrorhunt: but it'll get the defn from this header, right?
20:03rhuntGankro: yes
20:04Gankrorhunt: same story for function signatures?
20:04rhuntGankro: yes
20:06GankroThis is a surprisingly pleasant dev experience
20:16GankroCrashing with "Could not find symbol "glColorP4ui" by glcontext" -- I'm guessing this means some shaders updates are missing?
20:22rhuntGankro: sounds like a problem loading GL to me, what platform are you on?
20:22Gankromacos
20:23Gankro"WebRender - OpenGL version new 4.1 INTEL-10.24.45"
20:30rhuntGankro: did you add a call to glColor?
20:30Gankrorhunt: not to my knowledge
20:32rhuntGankro: weird
20:32rhunthave you gotten webrender running without any changes?
20:33Gankrorhunt: yes; I was looking at the profiler and everything
20:33Gankrorhunt: clean build time?
20:35rhuntGankro: it never hurts :) but i'm not sure what's going on
20:36Gankrorhunt: could be emergent behaviour from some memory corruption :shrug:
21:02Gankrorhunt: ah dang, still crashes
21:03rhuntGankro: what kind of changes have you made? we really shouldn't need to load this gl function
21:04rhuntGankro: a diff might help, you can use https://pastebin.mozilla.org/
21:04Gankrorhunt: hold on I'm gonna rebuild kats' branch to make sure that's still working
21:05Gankro(the webrender side of the changes is https://github.com/servo/webrender/pull/1124 )
21:05GankroAnd then fixing up a couple places in the gecko bindings to reflect the changes to signatures/layouts
21:16GankroFascinating
21:17Gankrothat build spews the same errors, but launches successfully
21:22GankroI need to clean up the webrender changes anyway, so I'm gonna call it a day
23:37pulsebotCheck-in: https://hg.mozilla.org/projects/graphics/rev/78699f5c92ca - Kartikaya Gupta - Bug 1357189 - Some reftests marked as fails-if(webrender) are actually passing with the pre-existing fuzziness annotations. So mark them passing. r=rhunt
18 Apr 2017
No messages
   
Last message: 70 days and 13 hours ago