<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/121638/">https://git.reviewboard.kde.org/r/121638/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 23rd, 2014, 1:38 a.m. UTC, <b>Rohan Garg</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;">Looks sensible to me, unless someone can come up with a better reason not to do this ( I couldn't find documentation anywhere about GST_VIDEO_NE, except for inside gst code itself, and no one, not even gst seems to be using this. )</p></pre>
</blockquote>
<p>On December 23rd, 2014, 7:43 a.m. UTC, <b>Harald Sitter</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;">But but but you used it? :P
http://lists.kde.org/?l=kde-commits&m=136482135607986</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">GST_VIDEO_NE supposedly stands for native endian giving the native endian type "RGB_LE" or "RGB_BE" depending on platform.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I do wonder if that line actually makes sense though... the original gstreamer0.10 version was:
+ gst_caps_new_simple("video/x-raw-rgb",
+ "bpp", G_TYPE_INT, 24,
+ "depth", G_TYPE_INT, 24,
+ "endianness", G_TYPE_INT, G_BIG_ENDIAN,
+ "red_mask", G_TYPE_INT, 0xff0000,
+ "green_mask", G_TYPE_INT, 0x00ff00,
+ "blue_mask", G_TYPE_INT, 0x0000ff,
+ NULL);
which is a far way from the code we have now. In particular the lack of bits per peixel being defined doesn't make me too excitied about the new version. The buffer of that element is later loaded into a QImage RGB24 (888) so there is really a very strict requirement on the output bpp.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyway, I'd feel more comfortable with Dan having a look and sprinkling us with wisdom. The blue tint issue a while ago also had to do with bad endianess.</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;">Oh, and, all that being said... I am fine with the review, someone just needs to make sure that Dan actually looks at the snapshot code in general ^^</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">slightly less broken is still better than entirely broken I say.</p></pre>
<br />
<p>- Harald</p>
<br />
<p>On December 23rd, 2014, 1:28 a.m. UTC, Albert Astals Cid 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 Phonon, Phonon Backends and Rohan Garg.</div>
<div>By Albert Astals Cid.</div>
<p style="color: grey;"><i>Updated Dec. 23, 2014, 1:28 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
phonon-gstreamer
</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;">I have no clue about gstreamer, but this works and the other not :D</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;">https://bugs.kde.org/show_bug.cgi?id=342140 works again</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>gstreamer/videowidget.cpp <span style="color: grey">(5dd35cb)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121638/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>