Skip to content

Improve handling of ffmpeg output #87

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Improve handling of ffmpeg output #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58ae63a
fix https://github.com/BotBlake/jellybench_py/issues/70
gingerbreadassassin Mar 17, 2025
2eedd22
fix KeyError: 'speed' when ffmpeg does not print 'speed'
gingerbreadassassin Mar 17, 2025
d7611d2
Fix KeyError: 'rss_kb'
gingerbreadassassin Mar 18, 2025
1f702bf
Merge branch 'develop' of https://github.com/BotBlake/jellybench_py i…
gingerbreadassassin Mar 19, 2025
35b4d43
hack missing workrss
gingerbreadassassin Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions jellybench_py/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,8 @@ def benchmark(ffmpeg_cmd: str, debug_flag: bool, prog_bar, limit=0) -> tuple:
result = {
"max_streams": max_pass,
"failure_reasons": failure_reason,
"single_worker_speed": max_pass_run_data["speed"],
"single_worker_rss_kb": max_pass_run_data["rss_kb"],
"single_worker_speed": max_pass_run_data.get("speed", 0),
Copy link
Owner

@BotBlake BotBlake Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sets speed to zero - The Client may try to downscale the ammount of workers, or not mark this run instantly as failed.
Additionaly, why is this access protected in core.py? Shouldn't it be ALWAYS written to in worker.py in the first place?

In fact, this PR already introduces the handling of this value in worker.py - this is just duplicated code.

This should not be done this way.
Specifically the lack of this value indicates a greater issue.
The run should be marked as failed.

"single_worker_rss_kb": max_pass_run_data.get("rss_kb", 0),
Copy link
Owner

@BotBlake BotBlake Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sets lasts run rss_kb to zero if not available - This has no effect on the runtime, but on the results.
Its just FALSE that the single worker_rss was zero.
Additionaly, why is this access protected in core.py? Shouldn't it be ALWAYS written to in worker.py in the first place?

Even though this does not introduce greater issues, the lack of this value indicates there are other issues to be solved.

}
if prog_bar:
prog_bar.update(status="Done", workers=max_pass, speed=f"{last_speed:.02f}")
Expand Down
7 changes: 6 additions & 1 deletion jellybench_py/worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ def workMan(worker_count: int, ffmpeg_cmd: str, passed_logger: Logger) -> tuple:
workrss = float(
rssline[1].split("=")[-1].replace("kB", "").replace("KiB", "")
) # maxrss
else:
workrss = 0
Copy link
Owner

@BotBlake BotBlake Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sets lasts run rss_kb to zero if not available - This has no effect on the runtime, but on the results.
Its just FALSE that the single worker_rss was zero - sure, WE know it, but the server does not when it tries to interpret the data.

Even though this does not introduce greater issues, the lack of this value indicates there are other issues to be solved.


if re.match(r"^bench: utime", line):
timeline = line.split()
Expand All @@ -134,7 +136,10 @@ def workMan(worker_count: int, ffmpeg_cmd: str, passed_logger: Logger) -> tuple:
new_line = line.split()
frames.append(int(float(new_line[0].split("=")[-1])))
framerates += int(float(new_line[1].split("=")[-1]))
speeds.append(float(new_line[6].split("=")[-1].replace("x", "")))
speed = new_line[6].split("=")[-1].replace("x", "")
if speed == "N/A":
speed = 0
speeds.append(float(speed))
Copy link
Owner

@BotBlake BotBlake Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the speed value doesnt exist at all instead of being written to with "N/A"?
Then converting to a float would not be possible.
Thats specifically the issue here.

Also its not clear what causes ffmpeg to not report this value.
Imo it points to other issues!

Anyways_
The results of ANY run that wasnt abled to gather valid result data from ffmpeg should NEVER be marked as healthy!
Having valid result data would indicate this!

Instead the run should be marked as unhealthy, by returning the failure.

lineAmmount = len(framelines)
if lineAmmount == 0:
lineAmmount = 1
Expand Down