Runtime estimation problem

[yippieyaray]

Hi Tom!

There is a problem estimating the runtime (estimate_runtime) if the number of remaining frames is equal or below the increment.

The reason is that the for-loop in line 182 uses a wrong result variable named outputGuess instead of output:

for length, outputGuess in [(days, "d"), (hours, "h"), (minutes, "m"), (seconds, "s")]:

If the calculated seconds is 1 or below, the loop runs to the end and the variable output keeps the value "guess".

The subsequent if-block steps to line 196 (call stack see below)

Changing the line 182 to the following line fixes the error: for length, output in [(days, "d"), (hours, "h"), (minutes, "m"), (seconds, "s")]

Regards, Ray

call stack:

Feb 21 11:48:00 raspberrypi python3[1364]: Traceback (most recent call last): Feb 21 11:48:00 raspberrypi python3[1364]: File "/home/pi/SlowMovie/slowmovie.py", line 378, in Feb 21 11:48:00 raspberrypi python3[1364]: logger.info(f"This video will take {estimate_runtime(args.delay, args.inc>Feb 21 11:48:00 raspberrypi python3[1364]: File "/home/pi/SlowMovie/slowmovie.py", line 196, in estimate_runtime Feb 21 11:48:00 raspberrypi python3[1364]: raise ValueError Feb 21 11:48:00 raspberrypi python3[1364]: ValueError

[qubist]

Hi @yippieyaray, and thanks for your bug report. Would you please create a PR for this fix so we can test and merge it?

[weow147]

interesting... updated. I'll keep an eye on it.

[qubist]

FWIF, I propose the following change to the estimate_runtime function, which fixes the issue you bring up, but also preserves code readability a bit better IMO than changing outputGuess

    if output == "guess":
        # Choose the biggest units that result in a quantity greater than 1
        # If no unit results in a quantity greater than 1, choose seconds
        for length, unit in [(days, "d"), (hours, "h"), (minutes, "m"), (seconds, "s")]:
            if length > 1 or unit == "s":
                return estimate_runtime(delay, increment, frames, unit)

(Note I renamed outputGuess to unit cause it makes more sense!)

Another alternative is as follows:

    if output == "guess":
        # Choose the biggest units that result in a quantity greater than 1
        for length, unit in [(days, "d"), (hours, "h"), (minutes, "m")]:
            if length > 1:
                return estimate_runtime(delay, increment, frames, unit)
        # If no unit results in a quantity greater than 1, choose seconds
        return estimate_runtime(delay, increment, frames, "s")

@yippieyaray, what do you think is the best quality solution? Welcoming your feedback. Feel free to make a PR if you'd like, or say the word and I can create one.

Thanks again!

[robweber]

The point of the "guess" type is to return the unit that makes sense given the run time remaining. In this case we end up with less than 1 second so none of the time units make sense. Why not just return "0 seconds"?

I also noticed the mention from another project (slow-movie) that seems to be a mashup of this project and support for IT8951 screens. I was unaware of that project but if anyone is watching from here it might be worth a collaboration on incorporating those changes downstream to SlowMovie. We've had numerous requests for that driver type and it's not that we're unwilling to add it but rather we, as of yet, have not found anyone to actually take on the task of adding it.

[yippieyaray]

Crash was fixed with my changes in „Estimate runtime refactoring“. PR was created.

[qubist]

@robweber If the remaining time is less than 1 second, seconds is still the appropriate unit:

For example: 0.5 second(s)

EDIT: which is how #117 implements it I think