<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121641/">https://git.reviewboard.kde.org/r/121641/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 6th, 2015, 1:22 a.m. CET, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Mpris specifications does not really target videos; plus I can't find a video player that would actually support this, makes me kinda doubt the usefulness of this (and as said couple times before, please discuss features first, we hate turning down code that has been already written). What player did you test this with?</p></pre>
</blockquote>
<p>On January 6th, 2015, 2:26 a.m. CET, <b>James Smith</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Mpris2 is player-agnostic, the xesam:url is included with dragon and vlc metadata, so the only drawback is accurate server mimetypes in the streaming case. Mpris2 does do well at what the player can pass along, there are some video formats that do include metadata (track / artist for video). There is also a patch (#121387) that simply ignores player tracks (audio or video) if there is insufficient metadata.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One of the Tears of Steel Blender Foundation streams (metadata + mime) (http://media.xiph.org/mango/tears_of_steel_1080p.webm) works with Dragon (Phonon+GStreamer) and a few Wikipedia .ogv (e.g. https://upload.wikimedia.org/wikipedia/commons/7/7d/Vela_Pulsar_jet_seen_by_Chandra_Observatory.ogv) have metadata, however their servers don't serve the correct mimetype.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't know if I'd call this a feature, more of a bugfix in tandem with #121387; video AND bad metadata should be simply ignored in the best of cases due to how bad the status messages can look when there isn't sufficient information. This is probably the best possible alternative.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The problem(s) to solve, are that video is not to be mislabelled as audio, and that deficient metadata also doesn't interfere with how "complete" the status message is.</p></pre>
</blockquote>
<p>On January 6th, 2015, 2:31 a.m. CET, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One of the Tears of Steel Blender Foundation streams (metadata + mime) (http://media.xiph.org/mango/tears_of_steel_1080p.webm) works with Dragon (Phonon+GStreamer) and a few Wikipedia .ogv (e.g.
https://upload.wikimedia.org/wikipedia/commons/7/7d/Vela_Pulsar_jet_seen_by_Chandra_Observatory.ogv) have metadata, however their servers don't serve the correct mimetype.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, that's mostly my concern. This will work with one or two players (as much as dragon is a real player) and about dozens of videos, for the rest it will just be disabled. I don't think it's really worth the maintenance overhead that we'd put on ourselves by adding that much complexity for virtually zero gain, sorry...</p></pre>
</blockquote>
<p>On January 6th, 2015, 7:05 a.m. CET, <b>James Smith</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">+50 loc to parallel the audio functionality and sort the metadata and +18 for the UI isn't that bad. The more players that support video metadata the better, IMO. I was surprised to find Dragon playing a video with mpris2 metadata. The only problem I see is the most common complaint from users might be that the video status message becomes "Now Listening to"..., for videos with passable metadata and a poorly configured server which is a potential complaint already, but surely a user properly setting "Now watching"... leaves every video with passable metadata properly labelled, irregardless of the server mimetype. I don't see any way to get around that, while players with no useful information should be prevented from interrupting status message output by default.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Leaving every video url with no useful information (and hence ignoring video by default) is the only other alternative, but that's still about a quarter of the loc, and it's a smaller stretch to just finish video support for now Playing. Or sooner or later video metadata hits its stride and catches someone wondering why video is completely ignored, when early metadata implementations were already good enough for mpris2 to pass along title / artist information, if not for the second problem of bad or lacking server mimetype configurations. Mpris2 the metadata interface is "Good Enough" for video metadata, now Playing has to support what the specification is capable of, and it's not really capable of ignoring videos without quite some complexity. The Internet has to catch up with a) proper mimetype configurations (while there are approved RFC's for streaming Ogg video, some providers haven't configured for Ogg video yet, but support the application/ogg mimetype) b) more content that has video metadata described, meaning also more ways of adding metadata to supported formats, and more players/libraries that support common video metadata specifications c) more formats that describe video metadata. Aside from the mime type catchup (which as a gripe isn't really limited to now playing or telepathy kde specifically), common mpris2 video metadata is not too far from being realized. Video players' responsibilities include filling the mpris2 metadata if it's there, and that's fairly hard to screw up for either audio or video.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why is +69 loc, too hard to maintain? The code to ignore videos would be at least +20 loc. There's really only an additional +50 loc from avoiding prefixing video content with "Now Listening to" by default (or at all), to properly prefixing both video and audio content separately. The output cleanup patch comes in bigger at +167 loc, but that's +110 lines of xml ui additions.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or sooner or later video metadata hits its stride and catches someone wondering why video is completely ignored</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If history is any indication, then this will just not happen. I'll be happy to reconsider though when we start getting dozens of bug reports about this.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The Internet has to catch up with a) proper mimetype configurations</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is YouTube age. There will be no catching up I'm afraid.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">b) more content that has video metadata described</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have about 73 various videos here from various sources, ranging from movies to music videos to documentaries, not a single one has any metadata on it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also just looking at the actual spec, yes it can theoretically handle video metadata, but even the basic set of metadata tags is strongly audio oriented, just naming a few: mpris:artUrl xesam:album xesam:albumArtist xesam:audioBPM xesam:composer xesam:discNumber xesam:lyricist xesam:asText --there's no single one clearly video oriented mentioned in the spec itself (yes I know the whole ontology is huge, but the specification does not mention any other even once). Which leads me to believe that video in mpris is more like "yeah, it can fit, but it's not our target".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">http://www.freedesktop.org/wiki/Specifications/mpris-spec/metadata/</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why is +69 loc, too hard to maintain?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Because code tends to break. Code tends to bitrot if not tested properly (and this wouldn't be). Code needs to be ported from time to time. Code with lots of features is more complex than simple code and always takes more time to understand properly. The less code we have, the better. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But it's not about the code itself. This is a thing that would be used by about 3 users once in their lifetime when they come across a video with proper metadata. And as a project maintainer, it's my job to decide if that's worth adding code for or not. Our target is unfortunately a simple~ish IM client, not swiss-army-knife-can-do-it-all IM client. And I really remain convinced that if someone wants to set a status about watching a movie (also nobody does that anymore anyway), they can simply write "I'm watching this awesome movie with Scarlett Johanson" as their status message.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On January 4th, 2015, 6:49 a.m. CET, James Smith wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Telepathy.</div>
<div>By James Smith.</div>
<p style="color: grey;"><i>Updated Jan. 4, 2015, 6:49 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ktp-kded-module
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Video data-specific support for now Playing plugin.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">None.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>CMakeLists.txt <span style="color: grey">(4a8aa4140e7fcda2fa36437cae98b1129f484fb4)</span></li>
<li>config/telepathy-kded-config.cpp <span style="color: grey">(9c686d1ca54c277e4ef5cfe95232a150f6ea102b)</span></li>
<li>config/telepathy-kded-config.ui <span style="color: grey">(93c06dc74b4dcb37e0473d0debfb5e738a24afa9)</span></li>
<li>telepathy-mpris.h <span style="color: grey">(05b77c90a50372fd9ed66bde0ab8a287caf34b51)</span></li>
<li>telepathy-mpris.cpp <span style="color: grey">(ee0e622c68bdd156e45914f542d2fe13f0ddb610)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121641/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>