<table><tr><td style="">TallFurryMan added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D20029">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20029#inline-112532">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wreissenberger</span> wrote in <span style="color: #4b4d51; font-weight: bold;">scheduler.cpp:1455</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">The problem occurs in line 4675 (sic!):<br />
<tt style="background: #ebebeb; font-size: 13px;">if (currentJob->getRepeatsRemaining() == 0)</tt><br />
when trying to find the next job. <tt style="background: #ebebeb; font-size: 13px;">evaluateJobs()</tt> is called before and sets <tt style="background: #ebebeb; font-size: 13px;">currentJob</tt> to <tt style="background: #ebebeb; font-size: 13px;">nullptr</tt> if no jobs are left over to be executed.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Then a fix is needed inside findNextJob, or if findNextJob assumes currentjob cannot be nullptr, there should be a debug assertion, and the call of findNextJob should be restricted.<br />
HOWEVER, I agree evaluateJobs must not change the current job, but that should not be part of this differential.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20029#inline-112799">View Inline</a><span style="color: #4b4d51; font-weight: bold;">scheduler.cpp:1952</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> if (jobEvaluationOnly || state != SCHEDULER_RUNNIG)
</div><div style="padding: 0 8px; margin: 0 4px; "> {
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The whole block after this should not be part of the evaluation I think as it does more than that. It should be part on the execution of the schedule. But let's keep this for another time.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20029#inline-112798">View Inline</a><span style="color: #4b4d51; font-weight: bold;">scheduler.cpp:2003</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> appendLogText(i18n("No jobs left in the scheduler queue after schedule cleanup."));
</div><div style="padding: 0 8px; margin: 0 4px; "> setCurrentJob(nullptr);
</div><div style="padding: 0 8px; margin: 0 4px; "> jobEvaluationOnly = false;
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">There is another one here that could cause your dereference. Still findNextJob should get a fix, eventually use the whole enclosing block in the future.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20029#inline-112597">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wreissenberger</span> wrote in <span style="color: #4b4d51; font-weight: bold;">scheduler.cpp:5212</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This is irrelevant, since the number of completed frames is only changed in the case that we remember the job progress (see lines 5266ff). In theory we could leave it, but it is not used in the case that we do not remember the job progress.</p>
<p style="padding: 0; margin: 8px;">This together with lines 5266ff resolve the second bullet point of 1. In the old implementation, the number of completed frames has been added for each of the sequence job entries and counted the same value multiply.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I still don't understand : capture_completed is used in areJobCapturesComplete to determine whether captures are needed, plus it may happen that some frames but not all are already captured when executing that block, even when not remembering progress. Removing this considers everything is still needed, which isn't true.</p>
<p style="padding: 0; margin: 8px;">Unless what you say is that we are counting the SCHEDULER job frame count as many times as there are SEQUENCE jobs, and that I agree is completely wrong! Good spot!</p>
<p style="padding: 0; margin: 8px;">So it happens that we cannot estimate the duration of a Scheduler job when not remembering progress? Couldn't we use the frame map?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D20029#inline-112599">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wreissenberger</span> wrote in <span style="color: #4b4d51; font-weight: bold;">scheduler.cpp:5268</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">An assertion does not make sense for the else branch, since in this case the number of completed frames is already set in the SequenceJob.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">OK to not change the job count if not remembering progress.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R321 KStars</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D20029">https://phabricator.kde.org/D20029</a></div></div><br /><div><strong>To: </strong>wreissenberger, mutlaqja, TallFurryMan<br /><strong>Cc: </strong>kde-edu, narvaez, apol<br /></div>