User:Catrope/Extension review/TimedMediaHandler

Revisions

• This review is as of r84443; line numbers refer to that rev unless specified otherwise.
• r84444: tweaked grammar in doc comment
• r84452: add some /* @embed */
• r84453: some indentation fixes (spaces->tabs mostly)
• r84945: Most of the bellow mentioned issues have been addressed, getting things ready for media and js review. Mdale 04:39, 29 March 2011 (UTC)

WMF deployment questions

• What kind of thumb URLs does this produce? The WMF 404 handler can already deal with OggHandler's thumb URLs, which look like thumb/a/a1/Foo.ogg/mid-whatever.whatever or Foo.ogg/seek=123-whatever.whatever , where ogg may also be ogv or oga. The thumbnails on prototype seem to stick to this format, but then I'm not sure what will happen in Firefox 4 with WebM support. Will this extension need any new thumb URL formats to be handled when deployed to WMF?

  Where do we hook in to update this? .. The "thumbs" path from Timed Media Handler is identical to OggHandler, except we also support webm "source" files. so thumb/a/a1/Foo.ogg/mid-whatever.whatever should work. NOTE currently neither OggHandler or TimedMediaHandler supports scaled image thumbnails both return the image as the size of the source video regardless of transform request. I will look into adding support for small thumbnails into TimedMediaHandler

  Added scaled thumbnail support in 84561 Mdale

  Thanks. The code I was talking about is in /trunk/tools/upload-scripts/thumb-handler.php and is WMF-specific (for thumb generation upon 404).

• Does this extension obsolete/replace Extension:OggHandler?

  Yes this would replace OggHandler. Parts of OggHandler are included in TimedMediaHandler.

Stuff I need help with reviewing

• Basically all of the JS; there's a shit ton of it. Maybe get Neil to help, or just do a skim for security rather than a 'real' review
• The media backend stuff is kind of over my head, would like Tim or Bryan to look over it

TimedMediaHandler.php

• Configuration should be near the top of this file. Right now it starts all the way down on line 72

  done in r84562

• Lines 18-22: the magic behavior of adding $wgTimedMediaHandlerFileExtensions to $wgFileExtensions (in this file) and $wgExcludeFromThumbnailPurge (in TMHHooks.php)
  • can be accomplished with array_merge()

    done r84563

  • doesn't work for any extensions added to that var in LocalSettings.php

    we don't want extensions adding to this var for now.. maybe in the future we support other formats but for now just a variable specific to tmh. --21:40, 22 March 2011 (UTC)Mdale

    • if this is intended to not work, that should be documented and it should be made clear this var is not a config var
    • if this is intended to work, the code for merging this var into $wgFileExtensions should be in an extension function and the var should be documented as a config var and put in the config section
• Lines 33-36: messing with include_path should only be done if it's really, really, really necessary. Since it seems to do this for a PEAR module I can imagine that it might indeed be needed, but if there's a way this could be done using regular autoloading or whatever that would be greatly preferable.

  This is copied from Tim's OggHandler, Will wait for comment from him before removal

• Lines 101-104: inconsistent casing between $wgffmpeg2theoraLocation and $wgFFmpegLocation

  fixed case in r84564

TimedMediaHandlerHooks.php

• Lines 34-49: you can move the resources/ part into the base path if you like, but either way is fine

  I prefer to include the /resources path so that its explicit where the resources are coming from. --Mdale 00:42, 23 March 2011 (UTC)

  Six of one, half a dozen of the other :) --Catrope 14:46, 23 March 2011 (UTC)

• Lines 68-103: dynamic namespace number determination scares me, mostly because it makes adding a custom namespace have a really weird effect (all TimedText pages will appear to have been moved into the new custom namespace). Instead, choose a constant, high number like 700 and register it. This way you also don't need to dynamically look for the TimedText namespace in TextHandler.php, and the name "TimedText" will be localizable

  That sounds better. But I think commons is already using 102 for TimedText for example. But I will set register TimedText in 700 namespace, but keep the "search" in there for now until a better solution is found or explained to me. --Mdale 00:42, 23 March 2011 (UTC)

  A config var could work too, I guess. The search, however, is a major source of trouble. --Catrope 14:46, 23 March 2011 (UTC)

oky removed the search and switched to config in r84686 Mdale 22:46, 25 March 2011 (UTC)

Was really in r84773, looks good. --Catrope 17:05, 30 April 2011 (UTC)

ApiQueryVideoInfo.php

PHP 5.3, hurry! :) Patches for making ApiQueryImageInfo more extensible are welcome. Currently all we have AFAIK is the ApiAfterExecute hook; you'd have to hook into that, parse the result object, recreate the File objects from that and add your properties. It's not as much trouble as it may sound like, but it's not the nicest thing ever and doesn't fix the getPropertyNames() issue. While we wait for PHP 5.3, a nice hack could be to wrap those static functions in ApiQueryImageInfo in non-static member functions and extend those.

this is a little tricky todo with patches and keeping the code running on trunk, but certainly a welcomed addition if someone wants to do this. --Mdale 17:19, 29 March 2011 (UTC)

TimedMediaHandler_body.php

• Lines 13-15: according to the documentation in the base class, getImageSize() should at least return something (array or false)

fixed

• Line 21: wfLoadExtensionMessages() was deprecated like a year ago. It can safely be removed as you're obviously not shooting for compat with 1.15 (given that you're using ResourceLoader) :)

don't see that there.

• Lines 86-92: use getter ($parser->getOutput()) instead of private member access ($parser->mOutput)

switched to getOutput()

• Line 130: document what seconds2npt() does and what NPT stands for

added some documentation

• Lines 159, 169, 170 : superfluous pair of parentheses around the isset call

fixed.

• Line 174: isVideo is a boolean for sure (cause it's built using the ! operator) so you can just use !$options['isVideo'] or reduce the amount of indirection and just use isAudio() instead of indirectly using !!isAudio()

also fixed ( and all the above in r84774 )

TimedMediaTransformOutput.php

• Line 60: another wfLoadExtensionMessages
• Line 115: all of these globals are unused
• Lines 157, 158: superfluous parentheses around $sizeOverride

fixed in r84775 --Mdale 23:14, 25 March 2011 (UTC)

TimedMediaIframeOutput.php

• Lines 16-33: I think it's better to do this with ?action=embedplayer , because MW provides a clean way to override actions instead of your hackery with ArticleFromTitle (of all hooks). It would also save you from having to prevent MediaWiki from doing the default page view action using various hacks like you are now

'we' sort of agreed that its similar to "printable" view, and we wanted to keep the same embed code valid for both gadget and for mediawiki as it adopted see bug 25269 .. note if you pull up any video on commons today uses the http://commons.wikimedia.org/wiki/File:Folgers.ogv?embedplayer=yes convention when you press "share".

• Lines 27-29: if you want to prevent other output, $wgOut->disable() should be good enough. It's also cleaner than duplicating the end of index.php and blowing up profiling (lots of unclosed profiling sections!) in the process

fixed

• Line 46: why do you need a clean OutputPage object? I think you might no longer need this if you use a decent action override as mentioned above, but you should at least document why you're doing this

no need to do this I think. Switched to $wgOut

• Lines 83, 90: mixing $j and $

switched to $ ( all in r84777 )

WebVideoTranscode.php

• Line 130: A common strategy in MW seems to be to use tempnam() with wfTempDir() (Import.php does this), or have a configurable temp path in a $wg var (includes/media/Bitmap.php does this with $wgImageMagickTempDir

one advantage of putting the in progress encodes in a public place is it may revel information about why a particular stream failed.. But either way is fine with me. --Mdale 23:28, 25 March 2011 (UTC)

TimedText.loader.js

• Line 17: don't use $j, use $

replaced all $j with $ in r84778

• Line 33: abrupt end of comment mid-sentence

removed line

• Line 41: you're using a possibly asynchronous event to check for something synchronously? This scares me. What's going on here?

The callbacks should only happen synchronously and its part of the event / extend model to put everything into triggers and

• Does any of this stuff actually need to be a loader script? AFAICT it's not determining the dependencies of another module or anything, it's just setting stuff up. Is there any reason this has to happen early?

it is establishing timed text as a dependency of the player. This could let you avoid loading the timed text library for videos that had no-timed text included. These videos ( with or without text tracks ) can be dynamically added to the page, so we want to check at the loader stage. But I can how it can be confusing because by default in the Wikimedia setup we 'always' include the timed text library. I did move the EmbedPlayerNewPlayer binding to the timedText file since its not loader code in r84779 --Mdale 23:41, 25 March 2011 (UTC)

RemoteMwTimedText.js

• Line 26: missing semicolon. This'll work because of semicolon insertion, but that's a scary "feature" of JS that should not be relied upon
• Line 60: doesn't this mean that every player has the same ID? Isn't that invalid?

only one player per timedText page

• Line 113: userLanugage is misspelled

opps. dyslexia strikes again.

• Line 150: so the width part of the default size is respected but the height part isn't?

? We are just grabbing some details about the video from the server, for embedding the player.

• Line 184: metadata.length is never gonna be less than zero, so this loop aborts immediately

I need to do a larger refactor of this code. It was needed to make the TimedText "view" pages when this was all a gadget. But now I should move some of the page layout into php ( since its no longer a gadget and no longer has to rewrite the page ) --Mdale 23:48, 25 March 2011 (UTC)

TimedTextEdit.js

• Lines 5-30: why are there hardcoded English messages here? ResourceLoader takes care of i18n and message exports for you

removed in r84939

that rev is unrelated (but I marked it OK) --Catrope 21:21, 30 April 2011 (UTC)

• this file is huge so I didn't review the rest of it just yet

EmbedPlayer.loader.js

• Lines 97, 99: skinname is leaked to the global scope

opps fixed in 84780

also unrelated, but marked OK --Catrope 21:22, 30 April 2011 (UTC)

• Same as with the other loader script: I don't see why any of this needs to be in a loader script

this builds out the list of dependencies for the embed player. It needs to be in a loader script because we want it globally available. ie you should be able to call $('myVideo').embedPlayer() from any page state. When you call embedPlayer it checks things like the skin, configuration options, and client browser to load the correct set of decencies for a player. --Mdale 23:54, 25 March 2011 (UTC)