<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/112076/">https://git.reviewboard.kde.org/r/112076/</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 17th, 2014, 9:01 a.m. UTC, <b>Harald Sitter</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/112076/diff/9/?file=234120#file234120line868" style="color: black; font-weight: bold; text-decoration: underline;">src/app/mainWindow.cpp</a>
<span style="font-weight: normal;">
(Diff revision 9)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">MainWindow::keyPressEvent( QKeyEvent *e )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">868</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">m_stopScreenPowerMgmtCookie</span> <span class="o">==</span> <span class="o">-</span><span class="mi">1</span><span class="p"><span class="hl">)</span></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">868</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">m_stopScreenPowerMgmtCookie</span> <span class="o">==</span> <span class="o">-</span><span class="mi">1</span><span class="hl"> </span><span class="o"><span class="hl">&&</span></span><span class="hl"> </span><span class="n"><span class="hl">TheStream</span></span><span class="o"><span class="hl">::</span></span><span class="n"><span class="hl">hasVideo</span></span><span class="p"><span class="hl">())</span></span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">While super lovely, this unfortunately suffers from the problem I outlined in a comment for review5.
Essentially video availability is async and independent of state change. Namely hasVideo doesn't need to be true when entering PlayingState (in fact it mostly isn't with Phonon VLC). So doing the static hasVideo is not sufficient. Instead engineHasVideoChanged() needs to be utilized in some form or fashion (wired up to Phonon, slot is called whenever hasVideo changed).
My original comment from back then was:
What I just remembered though... Phonon dispatches this information as a signal (i.e. state changes to playing; 3 seconds later; signal for hasVideoChanged arrives indicating this is audio-only content), so checking hasVideo() at the time of state change can lead to wrong behavior (e.g. hasVideo=false; 3 seconds later; hasVideo=true).
So yes, likely engineHasVideoChanged() is where you want to have additional logic.
Perhaps something like this:
new function MainWindow::inhibitSleep() [example name :S; contains sleep logic from inhibitPowersave()]
new function MainWindow::releaseSleep() [contains release for sleep logic from releasePowersave()[]
new function MainWindow::inhibitScreenSleep() [contains screen power management from inhibitPowersave()]
new function MainWindow::releaseScreenSleep() [contains release for screen power management from releasePowersave()]
remove function MainWindow::inhibitPowersave() [release can be kept around as convenience function)
MainWindow::engineStateChanged(...) calls MainWindow::inhibitSleep() [assumption: we always want to prevent sleeping]
MainWindow::engineHasVideoChanged(...) it calls MainWindow::inhibitScreenSleep() if hasVideo=true, or calls MainWindow::releaseScreenSleep() if hasVideo=no [overall condition: state must be playing]
[on state change to !playing we continue to call releasePowersave(), which in turn calls both releseSleep() and releaseScreenSleep()]
ultimate result is then that we always prevent sleeping </pre>
</blockquote>
<p>On January 17th, 2014, 9:31 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;">I had a problem with having separate screen functions in engineHasVideoChanged() where Dragon was unstable and generally crashed a ton.
Fortunately with both backends I've tested (VLC and GStreamer) in the terminal I get a nice KNotificationRestrictions debug message that appears when the video stream starts, and also reappears when the stream is paused and resumed. I think the original problem was that I was not requesting that hasVideo apply to KNotificationRestrictions.</pre>
</blockquote>
<p>On January 17th, 2014, 9:38 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;">It's a timing problem, whether or not it will fail entirely depends on timing.
https://projects.kde.org/projects/kdesupport/phonon/phonon-vlc/repository/revisions/master/entry/src/mediaplayer.cpp#L262
As you can see hasVideo changes are in no way bound to states. Yet the proposed diff will only evaluate hasVideo on state changes, as if it were tied to a state change. But regardless it is not, so the proposed diff is not correct.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't know if calling in engineHasVideoChanged() is the best solution either, given the crashes and general unstableness. Possibly putting a single shot timer to call the inhibitPowerSave function after a 5-second timeout, but I don't really like that solution for a problem that 1) may or may not be complained about in the future and 2) that tested probabilistically nil as a practical concern.
I can't validly confirm that a three second delay does happen to cause Dragon to 1) only some of the time (as opposed to all the time) fail triggering a screen saver suppression immediately 2) irresponsibly trigger a screen suppression immediately anyhow, despite having a delay.</pre>
<br />
<p>- James</p>
<br />
<p>On January 16th, 2014, 11:26 a.m. UTC, James Smith wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Multimedia and Harald Sitter.</div>
<div>By James Smith.</div>
<p style="color: grey;"><i>Updated Jan. 16, 2014, 11:26 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
dragon
</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;">Fixes an issue where playing audio content keeps the screen awake. Video content is supposed to, but with the new play URL button, dragon should allow the monitor to turn off.</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;">Compile, run-test</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>src/app/mainWindow.cpp <span style="color: grey">(f850820)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/112076/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>