Architecture meetings/RFC review 2014-05-21
Time: 2100 UTC
Date: Wednesday, 21 May, 2014
Place: #wikimedia-office
Agenda
editTo be discussed:
Summary & logs
editMeeting summary
edit- LINK: https://www.mediawiki.org/wiki/Architecture_meetings/RFC_review_2014-05-21 (sumanah, 21:00:10)
- square bounding boxes (sumanah, 21:00:31)
- LINK: https://www.mediawiki.org/wiki/Requests_for_comment/Square_bounding_boxes (sumanah, 21:00:39)
- LINK: https://gerrit.wikimedia.org/r/#/c/123683/ cscott is looking to get this merged :-) (sumanah, 21:00:45)
- AGREED: to +2 https://gerrit.wikimedia.org/r/#/c/123683/ (sumanah, 21:06:42)
- typesafe enums (sumanah, 21:06:56)
- LINK: https://www.mediawiki.org/wiki/Requests_for_comment/Typesafe_enums (sumanah, 21:06:59)
- Andrew Russell Green calls this "a facility that I've found really useful so far, and is used in other stuff that we'd like to propose moving to core :)" (sumanah, 21:07:04)
- AndyRussG today seeks "I guess a general path forward for any changes required to merge this to core, or an opinion on the feasability threof" (sumanah, 21:07:22)
- Andrew says: "I'm sure there are improvements to be made (for example, see MWalker's suggestions on the talk page), so if we can consider what would need to be done, and end up with something really nice that improves MW code quality, I think that'd be fantastic" (sumanah, 21:07:31)
- LINK: http://lists.wikimedia.org/pipermail/wikitech-l/2014-May/076605.html - there have been some questions/discussion on the mailing list by hashar & ^d & others (sumanah, 21:08:09)
- http://www.php.net/manual/en/class.splenum.php (AndyRussG, 21:13:59)
- AGREED: not gonna use SplEnum - it’s not available by default (brion, 21:23:49)
- ACTION: AndyRussG to benchmark setup and access performance (TimStarling, 21:24:50)
- square bounding boxes redux (sumanah, 21:25:25)
- <cscott> also, https://gerrit.wikimedia.org/r/#/c/133600/ is the more contentious part of this, i'd like to discuss that as well (sumanah, 21:26:35)
- ACTION: https://gerrit.wikimedia.org/r/123683 expected to be +2'd (cscott, 21:44:14)
- ACTION: cscott will work with greg-g to ensure no deployment hiccups (cscott, 21:44:25)
- ACTION: cscott will write a script to pre-generate thumbnails that will change size, to avoid scaler load (cscott, 21:44:40)
- LINK: https://gerrit.wikimedia.org/r/#/c/133600/ and https://bugzilla.wikimedia.org/63904 for those following along at home (cscott, 21:46:22)
- ACTION: cscott to do some git archeology to figure out the original author of upright, so that we can include him/her in the discussion (gwicke, 22:12:21)
- later discovered: Raimond Spekking <raymond@users.mediawiki.org>, git commit f8014e24e5d3292a555fc704b63bd14509e4f774 (http://mediawiki.org/wiki/Special:Code/MediaWiki/22305)
- ACTION: cscott to run the statistics and find out how many images would change size if we just ignored 'upright' entirely.
Meeting ended at 22:12:42 UTC.
Action items
edit- AndyRussG to benchmark setup and access performance
- https://gerrit.wikimedia.org/r/123683 expected to be +2'd
- cscott will work with greg-g to ensure no deployment hiccups
- cscott will write a script to pre-generate thumbnails that will change size, to avoid scaler load
- cscott to do some git archeology to figure out the original author of upright, so that we can include him/her in the discussion
- cscott to run the statistics and find out how many images would change size if we just ignored 'upright' entirely.
Full log
editMeeting logs |
---|
21:00:00 <sumanah> #startmeeting Discussion of square bounding boxes and typesafe enums| Channel is logged and publicly posted (DO NOT REMOVE THIS NOTE). https://meta.wikimedia.org/wiki/IRC_office_hours 21:00:00 <wm-labs-meetbot> Meeting started Wed May 21 21:00:00 2014 UTC and is due to finish in 60 minutes. The chair is sumanah. Information about MeetBot at http://wiki.debian.org/MeetBot. 21:00:00 <wm-labs-meetbot> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. 21:00:00 <wm-labs-meetbot> The meeting name has been set to 'discussion_of_square_bounding_boxes_and_typesafe_enums__channel_is_logged_and_publicly_posted__do_not_remove_this_note___https___meta_wikimedia_org_wiki_irc_office_hours' 21:00:05 <sumanah> #chair sumanah brion Tim-away 21:00:05 <wm-labs-meetbot> Current chairs: Tim-away brion sumanah 21:00:10 <sumanah> #link https://www.mediawiki.org/wiki/Architecture_meetings/RFC_review_2014-05-21 21:00:19 <sumanah> First, cscott :-) 21:00:22 <sumanah> since yours should be faster 21:00:31 <sumanah> #topic square bounding boxes 21:00:39 <sumanah> #link https://www.mediawiki.org/wiki/Requests_for_comment/Square_bounding_boxes 21:00:42 <sumanah> This should be quick 21:00:45 <sumanah> #link https://gerrit.wikimedia.org/r/#/c/123683/ cscott is looking to get this merged :-) 21:01:02 <sumanah> is that right C. Scott? 21:02:03 <sumanah> #chair sumanah brion TimStarling 21:02:03 <wm-labs-meetbot> Current chairs: Tim-away TimStarling brion sumanah 21:02:14 * brion double-checks the code 21:03:07 <brion> cscott: so as i understand this only affects the default case where something’s specified as a thumb without a size spec? 21:03:23 <brion> and sets a max height to match with the default thumb width 21:04:05 <brion> i’m pretty happy with that i think 21:04:06 <brion> TimStarling: ? 21:04:46 <TimStarling> I gave it +2 already, but then cscott changed the code substantially and I haven't gotten around to reviewing it again 21:04:59 <TimStarling> if it looks good to you, just give it +2 21:06:13 <brion> ok i’ve done so 21:06:18 <brion> let’s make sure it runs its tests and whatnot :) 21:06:28 <sumanah> Sweet 21:06:42 <sumanah> #agreed to +2 https://gerrit.wikimedia.org/r/#/c/123683/ 21:06:52 <sumanah> rock, we can move on I think. 21:06:56 <sumanah> #topic typesafe enums 21:06:59 <sumanah> #link https://www.mediawiki.org/wiki/Requests_for_comment/Typesafe_enums 21:07:04 <sumanah> #info Andrew Russell Green calls this "a facility that I've found really useful so far, and is used in other stuff that we'd like to propose moving to core :)" 21:07:22 <sumanah> #info AndyRussG today seeks "I guess a general path forward for any changes required to merge this to core, or an opinion on the feasability threof" 21:07:31 <TimStarling> it looks slow 21:07:31 <brion> ok so i think typesafe enums are cute and sometimes nice, but i don’t have a strong opinion 21:07:31 <sumanah> #info Andrew says: "I'm sure there are improvements to be made (for example, see MWalker's suggestions on the talk page), so if we can consider what would need to be done, and end up with something really nice that improves MW code quality, I think that'd be fantastic" 21:07:33 <cscott> sorry i'm late, what did i miss? ;) 21:07:39 <cscott> all of my rfc! 21:07:41 <TimStarling> cscott: brion merged your change 21:07:43 <sumanah> yes! 21:07:45 <brion> :D 21:07:51 <bawolff> and then jenkins bot rejected.. 21:08:00 <^d> As I said on the list, I'm not a fan of enums. I don't make use of them in languages that have them natively, and implementing them in userspace seems slow per Tim. 21:08:04 <cscott> probably because the release notes conflicted 21:08:05 <TimStarling> quick, rebase before he changes his mind again 21:08:09 <sumanah> #link http://lists.wikimedia.org/pipermail/wikitech-l/2014-May/076605.html - there have been some questions/discussion on the mailing list by hashar & ^d & others 21:08:27 <cscott> James_F thinks i should push to get this in 1.23 21:08:35 <brion> so there’s a few different kinds of enums in the universe i think 21:08:37 <cscott> but maybe we can return to discuss this after typesafe enums 21:08:44 <sumanah> cscott: yeah, sounds good to me. 21:08:44 <brion> one is ‘i really just need names for a couple flags’ 21:08:57 <brion> one is ‘i need user-friendly markers for id numbers that go in some API or database’ 21:09:00 <cscott> also, https://gerrit.wikimedia.org/r/#/c/133600/ is the more contentious part of this, i'd like to discuss that as well 21:09:03 <bawolff> Personally I'm not a fan of the syntax of having a $ in DayOfTheWeek::$TUESDAY (I know that's really superficial) 21:09:06 <TimStarling> in HHVM, class constants are probably very heavily optimised 21:09:07 <brion> and another is ‘i need a code-only marker for various extensible enum list' 21:09:10 * cscott will shut up while we discuss enums 21:09:21 <TimStarling> since the JIT knows what the value of the class constant is when it is generating machine code 21:09:28 <TimStarling> so it probably just uses the immediate value 21:09:56 <TimStarling> so I don't think the "$" is superficial, it makes it a hashtable lookup instead of an immediate machine code operand 21:10:11 <AndyRussG> Hmmmm :) 21:10:21 <brion> do we tend to use enums in perf-critical code paths? 21:10:25 <brion> (i’m thinking parser/sanitizer) 21:10:41 <AndyRussG> Also note that in this implementation nothing happens if the class isn't loaded 21:10:42 <cscott> i guess one question is: are we moving to enums for better readability of infrequently used code, or are we moving to enums because we want blazing fast performance on the hot paths through our codebase 21:10:54 <bawolff> I meant superficial from an aesthetic point of view 21:10:57 <cscott> because the SplEnum implementation is almost certainly faster. but not better for readability. 21:11:16 <cscott> (although it would be nice to get benchmarks to verify my intuitions) 21:11:34 <^d> SplEnum requires people installing a non-default pecl extension as bawolff pointed out on-list. 21:11:34 <AndyRussG> Re: peformance I do think it'd be a matter of a tradeoff, and some benchmarking would be cool 21:11:36 <cscott> i don't remember seeing wide-spread enums in the parser 21:11:39 <^d> That's a -1 from my POV for it. 21:11:58 <bawolff> ^d: I didn't point that out, but I agree I don't like that :) 21:11:58 <sumanah> AndyRussG: https://www.mediawiki.org/wiki/Performance_profiling_for_Wikimedia_code might help you do a bit of benchmarking. Maybe. 21:12:15 <cscott> well, a standalone microbenchmark would be my first pass 21:12:27 <cscott> just to get a rough order of magnitude for the different enums 21:12:40 <cscott> also, a standalone microbenchmark would probably be easier to run on HHVM 21:12:41 <^d> bawolff: Sorry, Tyler, not you. 21:12:47 <mwalker> a common usecase for enums is in function argument -- instead of having a bare TRUE / FALSE / NULL you have some explicit type 21:12:48 <AndyRussG> Definitely ... Still if it's a trade-off between an extremely small performance difference and better and typesafe code, I'd take the latter 21:13:11 <^d> mwalker: Well if you're passing boolean params to a function you're breaking coding conventions anyway :) 21:13:12 <AndyRussG> For me the ability to to typehint is really nice 21:13:49 <AndyRussG> The issue I have with SplEnum is it works quite differently from enums in other languages and is extra verbose 21:13:59 <AndyRussG> #info http://www.php.net/manual/en/class.splenum.php 21:14:31 <robla> I think Antoine may be the only one arguing for SplEnum, but I could be mistaken. Anyone arguing for it here? 21:14:41 <brion> not i, i don’t like splenum much 21:14:43 <^d> I don't think anyone's arguing for it. 21:14:53 <AndyRussG> Cool 21:14:55 <^d> I think Antoine was just like "hey maybe this?" not so much a strong advocate. 21:15:05 <brion> so i think there are two questions 21:15:14 <brion> one is: do we want to use this sort of facility widely? 21:15:25 <brion> the other is: when folks do want to use it, should they use a common base class from core? 21:15:31 <brion> -or implement separately in every ext? 21:15:35 <cscott> nope, i like the new code. but i'd like to get a little bit of insight into performance. 21:15:45 <cscott> so that we know whether to recommend it for hot paths or not 21:16:00 <cscott> gwiw, Parser->mOutputType is an enumerated type, but it's not on a hot path 21:16:44 <cscott> Parser::setFunctionHook takes an enumeration too, again now performance-sensitive 21:16:49 <TimStarling> __toString() should probably be included in the benchmark 21:17:04 <AndyRussG> I can do benchmarks and add them to the RFC 21:17:05 <TimStarling> since that suggested case syntax will call it 21:17:20 <^d> brion: We'd have to ask more people if they want to use it widely. 21:18:14 <AndyRussG> I think that most places where you use a class constant you could use something like this 21:18:44 <TimStarling> SplEnum is PECL? 21:18:47 <brion> so iiuc, basically proposal is to copy https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FCampaigns.git/e4714059330ace2c9c8457433919fb90ce1f4af5/includes%2FTypesafeEnum.php into core so people could use it 21:18:54 <TimStarling> that kind of rules that out doesn't it? 21:19:19 <brion> $ php maintenance/eval.php 21:19:19 <brion> > return class_exists('SplEnum'); 21:19:20 <brion> bool(false) 21:19:26 <^d> TimStarling: Yes. 21:19:29 <brion> yeah let’s kill SplEnum it’s not there by default 21:19:31 <AndyRussG> I think we could also add what mwalker suggests on the talk page, about setting a constant integer value for database, and some facilities for bitmask when desired 21:19:50 <gwicke> quick side question: do you plan to discuss square bounding boxes for upright as well in this meeting? 21:20:26 <^d> Already done. 21:20:29 <^d> brion merged. 21:20:40 <brion> gwicke: oh i think we forgot about that case — it might make sense to turn uprights into squares if the current patch doesn’t do that 21:20:44 <TimStarling> not for upright, just for default params 21:20:51 <sumanah> gwicke: We're returning to the topic after the enums chat because there's more to discuss, now that cscott is around. 21:20:52 <^d> Oh nvm, ignore me. 21:21:41 <sumanah> it's ok! we were done until we were not :) 21:22:36 <AndyRussG> re: the enums, I looked through a few other folks' implementations, seemed a lot of people are doing something similar, though I imagine it's not enough code to make worrying about an external library worth the effort 21:22:53 <TimStarling> it probably won't be a hashtable lookup in HHVM, btw, I think it will just be dereferencing and Variant unboxing 21:22:56 <cscott> square bounding boxes now? 21:22:59 <gwicke> brion, TimStarling, sumanah: thx 21:23:05 <cscott> i thought i could fix the rebase conflict before we got back to me 21:23:15 <cscott> but i'm multitasking poorly 21:23:27 <sumanah> are we done with the enums topic? can we have a couple #agreed or #action lines? 21:23:49 <brion> #agreed not gonna use SplEnum - it’s not available by default 21:24:10 <brion> i think we’re still kinda undecided on whether to put TypesafeENums class into core 21:24:20 <brion> and even more undecided on whether to change any code to use it 21:24:21 <AndyRussG> Is doing some benchmarks #agree-able? 21:24:31 <brion> sounds good. TimStarling ? 21:24:50 <TimStarling> #action AndyRussG to benchmark setup and access performance 21:25:00 <AndyRussG> cool! :) 21:25:03 * TimStarling likes #action 21:25:08 <brion> \o/ 21:25:12 <sumanah> sweet 21:25:24 <sumanah> ok, we can therefore move on to 21:25:24 <AndyRussG> mwalker: you mentioned some other use cases before? should we look at those? 21:25:25 <sumanah> #topic square bounding boxes redux 21:25:26 <MartijnH> from what TimStarling was saying earlier, HHVM benchmarks are also desirable? 21:26:03 <mwalker> well; if we dont want to use this for performance critical code then it doesn't make much sense 21:26:12 <mwalker> but I had suggested use cases of namespaces and permissions 21:26:32 <AndyRussG> MartijnH: I can also try HHVM benchmarks 21:26:35 <sumanah> #info <cscott> also, https://gerrit.wikimedia.org/r/#/c/133600/ is the more contentious part of this, i'd like to discuss that as well 21:27:01 <TimStarling> hhvm is pretty easy, now that they have packages on hhvm.com 21:27:17 <AndyRussG> Ah cool plums 21:27:22 <TimStarling> just run "hhvm -f benchmark.php" instead of "php benchmark.php" 21:27:46 <TimStarling> or "hhvm -v Eval.Jit=true -f benchmark.php" to be sure the JIT is enabled 21:27:48 <cscott> sigh rebasing fail, i managed to add https://gerrit.wikimedia.org/r/#/c/134734/, boo me 21:28:06 <gwicke> so I'm not convinced that upright has much of a use case left with square bounding boxes 21:28:29 <AndyRussG> TimStarling: thanks 21:28:30 <gwicke> the only use case would be resizing images relative to a user's default thumb size preference 21:29:18 <TimStarling> hmm 21:29:19 <gwicke> which might be better covered by introducing more semantic alternate image types 21:29:26 <TimStarling> you know this change that cscott is merging... 21:29:36 <TimStarling> will that make the site go down when it is deployed? 21:29:42 <cscott> what? 21:29:44 * TimStarling always thinks of these things too late 21:30:05 <TimStarling> well, how many thumbnails do you suppose will be regenerated? 21:30:10 <AndyRussG> mwalker: (aside: still enumish: got more details, maybe to put on the talk page or send elsewhere? thanks btw) 21:30:42 <cscott> TimStarling: the non-square ones, i guess. 21:30:44 <TimStarling> better make sure Greg and Sam know about it I guess 21:30:49 <cscott> we could probably pre-generate them. 21:30:57 <cscott> i could give you exact statistics 21:31:18 <cscott> i have a db of all figure usage on en/fr/it/de/.. wikis 21:31:30 <TimStarling> the question is whether the image scalers or swift backend will fall over when it is deployed to en or commons, due to increased rate of scaling operations 21:32:28 <cscott> ok, patches are rebased, so i can devote brain cells to discussion 21:32:41 <cscott> i think it would be a good idea to pre-scale as many images as possible 21:33:02 <cscott> i can generate a list of all images on enwiki (say) that would change size as well as the new size 21:33:16 <TimStarling> you think it won't be too difficult to generate that list? 21:33:18 <cscott> i should just be able to ask mw for those thumbs in the new size to seed them in swift 21:33:22 <cscott> no, less than an hour 21:33:27 <AaronSchulz> ^d: is mwgrep on the expanded text or the source text? 21:33:29 <TimStarling> ok, sounds good then 21:33:31 <cscott> it's a good segue to the *next* topic 21:33:47 <^d> AaronSchulz: Expanded. We don't store unparsed wikitext. 21:34:04 <^d> (yet. maybe?) 21:34:12 <cscott> since i wrote that code/assembled that db in order to show that https://gerrit.wikimedia.org/r/133600 would be safe 21:34:25 <cscott> see https://bugzilla.wikimedia.org/show_bug.cgi?id=63904 for a link to the code, and statistics 21:34:33 <cscott> that part of the patch affects far fewer images 21:35:49 <gwicke> I gave my opinion on that earlier in this meeting 21:35:54 <cscott> swift stuff lasts forever, right? so i can start requesting the new sizes for the thumbnails now, regardless of the deployment/merge time of the patch. 21:36:28 <TimStarling> yes 21:36:34 <cscott> let's sync up -- so are we still talking about 'thumbs with no explicit size should use a square bounding box (that's https://gerrit.wikimedia.org/r/123683) 21:36:41 <cscott> let's finish up that part 21:36:49 <cscott> the patch has been rebased 21:37:17 <cscott> my understanding is that someone will +2 it, and i will asap start a process to start requesting thumbs that change size, so that changeover will not be drastic 21:37:41 <cscott> and someone (?) will inform greg and sam, so we all know what's going on before the code goes live? 21:37:47 <TimStarling> last meeting we had "ACTION: cscott to patch upright to have a square bounding box, and use dumpGrepper to see whether this breaks too much (cscott, 21:59:55)" 21:38:10 <cscott> TimStarling: that's the next part. i'm just trying to make sure we're all on the same page regarding the first part. 21:38:12 <cscott> before we move on 21:38:51 <cscott> first part being "thumbs with no explicit size" (upright flag semantics unchanged) 21:39:21 <cscott> ok? my summary above is correct for the first part? 21:39:41 <cscott> sumanah: you want to add some # actions? 21:39:51 <TimStarling> greg-g: for changes that need to be deployed carefully, I'm meant to add you as a reviewer, right? 21:40:28 <gwicke> I think we pretty much agree that with the right preparations the move to square bounding boxes for bare thumbs is a good idea 21:40:46 <greg-g> TimStarling: that's good yeah, what are you thinking of? 21:40:50 <greg-g> ie: how carefully? :) 21:41:21 <greg-g> TimStarling: ah, I see in -dev 21:41:27 <TimStarling> load on swift and image scalers needs to be watched, since thumbnailing parameters will change 21:41:30 <cscott> well, at a minimum you should probably check with me to ensure that my job to pregenerate the new image thumbs has completed? 21:41:39 <AndyRussG> be vewy vewy quite 21:41:56 <AndyRussG> quiet 21:41:58 <TimStarling> if there is an overload, it should be rolled back 21:42:09 <greg-g> gotcha 21:42:35 <greg-g> cscott: can we chat post this discussion re deploy details? 21:42:37 <cscott> other than me writing a script to try to pregenerate images, is there any other possible deployment strategy to mitigate? 21:43:00 * sumanah wants to leave #action items to others to decide 21:43:11 <cscott> greg-g: sure. 21:43:52 <cscott> and maybe it should be flagged in the release notes more prominently that this will have affect on scaler load? 21:44:04 <greg-g> cscott: sounds good 21:44:14 <cscott> #action https://gerrit.wikimedia.org/r/123683 expected to be +2'd 21:44:25 <cscott> #action cscott will work with greg-g to ensure no deployment hiccups 21:44:40 <cscott> #action cscott will write a script to pre-generate thumbnails that will change size, to avoid scaler load 21:45:19 <cscott> ok, are we done with that then? 21:45:52 <gwicke> I believe so 21:45:54 <cscott> now we can start the screaming and yelling ;) 21:46:00 <gwicke> let's move on 21:46:06 <cscott> so the second part makes 'upright' simultaneously more and less useful 21:46:22 <cscott> https://gerrit.wikimedia.org/r/#/c/133600/ and https://bugzilla.wikimedia.org/63904 for those following along at home 21:46:32 <cscott> it makes 'upright' also use a square bounding box. 21:46:49 <cscott> so that it actually is useful for upright images, and doesn't require the user to manually specify the aspect ratio of the image 21:47:17 <cscott> Something like 0.75% of the images on enwiki use the 'upright' flag 21:47:26 <cscott> this would change the size of 0.04% of the images 21:47:42 <gwicke> it'd be a mis-named 'scale' option, which scales relative to the user / wikis's default thumb size pref 21:47:56 <cscott> since the number of images whose size changes is so small, it is proposed to reset the scale factor to 1.0 at the same time, which makes upright's semantics much less mysterious 21:48:19 <cscott> (changing the default scale changes about 110 image references on enwiki) 21:48:57 <cscott> yes, the semantics of upright would be pretty much "scale", and one might consider in the future adding an alias. but i'd rather upright go away entirely the the future. ;) 21:48:58 <subbu> cscott, how many on frwiki use that flag? 21:49:09 <cscott> it's in the bugzilla 21:49:28 <subbu> because i remember reading on ve-feedback page that upright is popular/encouraged on frwiki 21:49:49 <cscott> 17,870 of 1,242,985 images use 'upright' and the proposed change would alter the size of 1049 or 1053 of them. (the latter if the scale factor was changed to 1.0) 21:50:02 <cscott> on frwiki 21:50:42 <cscott> for dewiki, it's 38,624 of 1,786,779 and the size would be altered for 1963/1764 21:50:52 <cscott> while i'm copying numbers into chat ;) 21:51:06 <TimStarling> can anyone explain to me why upright isn't the stupidest thing ever? 21:51:09 <cscott> so deploying this one should be a breeze, at least. ;) 21:51:17 <subbu> :) 21:51:23 <gwicke> TimStarling, that'd be hard 21:51:31 <cscott> TimStarling: i think both gwicke and i agree that upright is stupid. 21:51:44 <TimStarling> who approved it? 21:51:49 <cscott> this patch makes it slightly less stupid. at least it has some sensible semantics (scales the default thumbnail size) 21:52:11 <cscott> instead of being some completely weird thing that i don't want to write a bunch of special-case code for in parsoid 21:52:19 <gwicke> to me the interesting bit is that there are very few uses of the scale factor 21:52:26 <cscott> (actually, that i've already written a bunch of special-case code for in parsoid, multiple times) 21:52:32 <TimStarling> the help says "When the height of an image in thumbnail is bigger than its width (i.e. in portrait orientation rather than landscape) and you find it too large, you may try the option upright=N, where N is the image's aspect ratio (its width divided by its height, defaulting to 0.75)." 21:52:39 <gwicke> most non-scale factor uses will basically be made unnecessary by square bounding boxes by default 21:52:50 <cscott> gwicke, no i think you are misinterpreting the statistics 21:53:10 <TimStarling> if you do that, calculate the aspect ratio and use that as the upright factor, that is equivalent to a square bounding box, correct? 21:53:40 <gwicke> cscott, you don't break out the explicit factors vs. those using the default 21:53:54 <gwicke> but in my experience the explicit factor is very rare 21:54:15 <TimStarling> but without an explicit factor, upright is just equivalent to multiplying the width by 0.75 21:54:28 <cscott> i think the ones which change size when the default scale factor is tweaked are the ones without an explicit scale factor. 21:54:28 <cscott> that's like 110 images on enwiki. 21:54:28 <cscott> so i think most upright images do actually have a scale factor. 21:54:28 <cscott> i can crunch the exact numbers for you if you like. 21:54:28 <cscott> TimStarling: i wrote that help text, i think. 21:54:33 <cscott> the previous help text was much less helpful, and stated behavior that differed from the implementation 21:55:01 <cscott> TimStarling: yes. multiplying the width by 0.75 is useful if all your images are either 4:3 or 3:4 21:55:18 <cscott> so upright w/o a scale factor gives you 4:3 images that are 'the right size' next to the rest of your 3:4 images 21:55:29 <cscott> are least that's my reverse-engineering of the logic 21:55:33 <cscott> *at least 21:55:40 <gwicke> afaik the factor was added later 21:55:48 <gwicke> which also explains the weird name 21:56:30 <gwicke> cscott, so I think it would be good to have data on how common the scale is actually set explicitly 21:56:30 <TimStarling> but images are typically not one of those two aspect ratios 21:56:43 <cscott> anyway, i think this patch is worth merging, because it affects very few images, and at least gives us semantics for 'upright' that don't require the user to manually compute aspect ratios. 21:57:14 <cscott> gwicke: i can compute that, but i'm curious why you want to know. 21:57:26 <gwicke> if there are few uses with explicit scale factors then I'd prefer to just deprecate upright 21:57:44 <gwicke> since it'll be much less useful 21:57:48 <TimStarling> upright with a specified width seems kind of pointless 21:57:51 <gwicke> and mis-named as well 21:57:56 <cscott> there are 37,185 uses of upright on enwiki 21:58:03 <TimStarling> since you could just multiply the specified width by the scale factor and omit upright 21:58:05 <cscott> we could deprecate it, but probably not without tool assistance 21:58:22 <cscott> upright is ignored if there is a specified with. 21:58:24 <cscott> *width 21:58:28 <cscott> there are parser tests for that 21:58:33 <TimStarling> right... 21:58:54 <gwicke> the original use case for upright will disappear with square bounding boxes 21:58:58 <cscott> it's only use is scaling the "default size" of the thumbnail (which might be user-specified) 21:58:58 <TimStarling> without a specified width -- at least it does something unique 21:59:34 <cscott> (and by user specified i mean in the user's preferences, not in wikitext) 21:59:41 <TimStarling> well, the question is whether people actually want square bounding boxes or if they want their images to be shrunk by some arbitrary factor 22:00:04 <TimStarling> the rounding to the nearest 10px is pointless, right? 22:00:25 <cscott> TimStarling: i think the real missing feature is a 'scale and crop' option that will give me an exactly XxYpx image if i ask for one, cropping the edges as needed. but that's a different patch set. 22:00:29 <TimStarling> it doesn't actually reduce the number of generated images, like it claims 22:00:42 <gwicke> the 0.75 default combined with the then-common 3:4 aspect ratio suggests that they wanted the equivalent of square bounding boxes 22:00:49 <cscott> it is probably pointless, yes. 22:01:37 <TimStarling> gwicke: but square bounding boxes already existed 22:01:44 <TimStarling> at least for specified widths 22:01:56 <gwicke> yeah, but not for a bare 'thumb' 22:02:18 <TimStarling> if that's what they wanted, it would have been trivial to implement 22:02:27 <gwicke> remember that upright is only used if there are no explicit dimensions 22:02:28 <cscott> since the user's preferred thumbnail size was variable, it was formerly impossible to get "a square bounding box of the user's preferred thumbnail size" 22:02:32 <TimStarling> at the place where the 0.75 factor is applied, the aspect ratio is loaded and trivially available 22:02:49 <cscott> i believe it was a short-sighted design originally 22:03:08 <cscott> that probably implemented what the user asked for, without pausing to think about what the user actually wanted. 22:03:16 * gwicke nods 22:04:14 <gwicke> if we were to design a scale option relative to the default size from scratch, how would we go about it? 22:04:33 <cscott> (the current patch 133600 includes the 'round width to 10px' behavior, but i would be happy to take that out.) 22:04:47 <TimStarling> well, I would call it "scale", not "upright", for a start 22:04:51 <cscott> gwicke: we wouldn't. we'd take scaling decisions out of the user's hands and add semantic classes for images instead 22:05:16 <gwicke> TimStarling, cscott: +1 to both of you ;) 22:05:22 <Cloakedcitizen> hey folks 22:05:33 <TimStarling> but I think we would want to consider very carefully whether such a feature is needed at all 22:05:35 <cscott> We can make scale an alias for upright w/o breaking the 37k existing uses 22:05:53 <cscott> even better, i could rename the option to 'scale' in the source and docs, and include 'upright' as the alias 22:06:06 <TimStarling> if someone asked me for it out of the blue, I would probably say no 22:06:15 <TimStarling> I probably did, that's why I don't know anything about it ;) 22:06:18 <gwicke> cscott, thumb|upright could just become a no-op with square bboxes 22:06:24 <cscott> but my personal preference is *not* to add scale, because i think the long term plan is to get rid of upright, not legitimatize it 22:06:49 <gwicke> the only interesting case would be upright with an explicit factor 22:06:53 <TimStarling> what about deprecating and ignoring the parameter to upright? 22:07:00 <cscott> gwicke: actually, with upright's scale factor set to 1.0 by default, making upright a no op would probably be not too different. 22:07:37 <cscott> i can re-run the numbers for (a) what if we just ignore 'upright entirely' and (b) what if we ignore the scale factor -- but isn't (b) equivalent to (a)? 22:07:45 <TimStarling> we should find out who requested this feature and who implemented it 22:07:52 <cscott> i think upright already requires 'thumb' to also be specified, or it has no effect. 22:08:03 <cscott> git blame will tell you that, i bet. 22:08:25 <gwicke> cscott, at the minimum we should probably ignore a bare upright with thumb 22:08:38 <gwicke> as the square bbox should take care of that 22:08:41 <cscott> i think setting the default upright scale factor to 1.0 has that effect. 22:08:44 <TimStarling> whoever it was should be included in the discussion 22:08:58 <cscott> (with my patch. without my patch, thumb|upright gives you a non-square bbox) 22:09:13 <gwicke> yeah, it works out to the same 22:10:17 <gwicke> so I think we'd all prefer to get rid of upright if possible, at least in the longer term 22:10:18 <TimStarling> we could deprecate upright completely, and start discussions with whoever requested the feature in the first place about what they really need 22:10:44 <TimStarling> but like I say, they should be included 22:10:48 <cscott> i'm trying to determine who that is from git-blame, but it's been hidden by layers of patches 22:11:01 <cscott> it was added before oct 2010, that's all i can say at the moment 22:11:41 <TimStarling> let's end this meeting, since we're out of time, we can talk about it later 22:11:55 <cscott> in the interest of moving things forward, i think that merging my patch would still be a good first step to deprecating upright. ;) 22:12:14 <TimStarling> noted 22:12:15 <cscott> since the scale=1.0 feature then makes bare upright a no-op 22:12:21 <gwicke> #action cscott to do some git archeology to figure out the original author of upright, so that we can include him/her in the discussion 22:12:42 <TimStarling> #endmeeting right after that: <cscott> #action cscott to run some additional statistics <cscott> anyway, i'll run the statistics and find out how many images would change size if we just ignored 'upright' entirely. <cscott> since i agree that's an even better way to make upright sane. 03:16:56 PM) cscott-free: Raimond Spekking <raymond@users.mediawiki.org> was responsible, on may 21 2007: git commit f8014e24e5d3292a555fc704b63bd14509e4f774 (03:17:38 PM) bawolff: cscott: http://mediawiki.org/wiki/Special:Code/MediaWiki/22305 (03:17:45 PM) cscott-free: Introducing new image parameter 'upright' and corresponding variable $wgThumbUpright. (03:17:45 PM) cscott-free: This allows better proportional view of upright images related to landscape images on a page without nailing the width of upright images to a fix value which makes views for anon unproportional and user preferences useless (03:17:45 PM) cscott-free: Usage: (03:17:45 PM) cscott-free: * [[Image:pix.jpg|thumb|upright|caption]] = Upright image will be scaled down by $wgThumbUpright (default 0.75, seems to me the best value) (03:17:45 PM) cscott-free: * [[Image:pix.jpg|thumb|upright=0.6|caption]] = Upright image will be scaled down by 0.6 (03:17:45 PM) cscott-free: Size of thumb is always rounded to full __0 px to avoid odd thumbsizes and spare the cache (03:17:45 PM) cscott-free: (03:17:45 PM) cscott-free: If used in combination with a width, upright will be ignored. (03:21:41 PM) TimStarling: cscott: no bug report referenced (03:21:48 PM) cscott-free: so.. do we want to schedule another rfc meeting for upright, and invite Raimond Spekking ? (03:22:06 PM) cscott-free: TimStarling: yes, i was poking around looking for that. mw didn't use code review back then, did it? (03:22:26 PM) TimStarling: I didn't realise it was so long ago, he probably doesn't care anymore (03:22:49 PM) TimStarling: yeah, so it was possible for things to sneak in without review (03:23:13 PM) TimStarling: there was a commits mailing list (03:23:34 PM) TimStarling: review basically depended on the "unread" status in brion's mail client (03:23:58 PM) sumanah: cscott: I say yes. Would you terribly mind checking in with Raimond about what time, in some upcoming M/W/F, he would be available? It's also fine to try to work this out on the mailing list which might be easier (03:24:14 PM) brion: it was a silly time |