D20029: Capture counting corrected

[Eric Dejouhanet]

TallFurryMan added inline comments.

INLINE COMMENTS

> wreissenberger wrote in scheduler.cpp:1455
> The problem occurs in line 4675 (sic!):
> `if (currentJob->getRepeatsRemaining() == 0)`
> when trying to find the next job. `evaluateJobs()` is called before and sets `currentJob` to `nullptr` if no jobs are left over to be executed.

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.
HOWEVER, I agree evaluateJobs must not change the current job, but that should not be part of this differential.

> scheduler.cpp:1952
>  
>      if (jobEvaluationOnly || state != SCHEDULER_RUNNIG)
>      {

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.

> scheduler.cpp:2003
>          appendLogText(i18n("No jobs left in the scheduler queue after schedule cleanup."));
>          setCurrentJob(nullptr);
>          jobEvaluationOnly = false;

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.

> wreissenberger wrote in scheduler.cpp:5212
> 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.
> 
> 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.

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.

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!

So it happens that we cannot estimate the duration of a Scheduler job when not remembering progress? Couldn't we use the frame map?

> wreissenberger wrote in scheduler.cpp:5268
> 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.

OK to not change the job count if not remembering progress.

REPOSITORY
  R321 KStars

REVISION DETAIL
  https://phabricator.kde.org/D20029

To: wreissenberger, mutlaqja, TallFurryMan
Cc: kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190326/37d96e4e/attachment-0001.html>