<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, 12:22 a.m. UTC, <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, 1:26 a.m. UTC, <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, 1:31 a.m. UTC, <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>
</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;"><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>
<br />
<p>- James</p>
<br />
<p>On January 4th, 2015, 5:49 a.m. UTC, 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, 5: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>