Jump to content

Architecture meetings/RFC review 2014-05-21

From mediawiki.org

Time: 2100 UTC

Date: Wednesday, 21 May, 2014

Place: #wikimedia-office

Agenda

[edit]

To be discussed:

  1. Requests for comment/Typesafe enums
  2. Requests for comment/Square bounding boxes

Summary & logs

[edit]

Meeting summary

[edit]
  • 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)


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

[edit]
Meeting 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
Retrieved from "https://www.mediawiki.org/w/index.php?title=Architecture_meetings/RFC_review_2014-05-21&oldid=2878303"