8000 [BUGFIX] Fix MINLENGTH for track.process by joelishness · Pull Request #1302 · automatic-ripping-machine/automatic-ripping-machine · GitHub
[go: up one dir, main page]
Skip to content

[BUGFIX] Fix MINLENGTH for track.process #1302

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[BUGFIX] Fix MINLENGTH for track.process #1302

wants to merge 3 commits into from

Conversation

joelishness
Copy link
Contributor

Description

Commit d087730 removed MINLENGTH from being passed to makemkv for single track.
As a result, makemkv would use default of 120s for such conditions.
If the user specifies MINLENGTH <120s, unexpected behavior occurs, such as jobs erroring out and jobs finishing without the desired titles.

I don't know if there is any particular reason for the original removal, so please confirm whether there is some case I haven't considered. But it resolved the issue in my testing.

Fixes #1299

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Ripped DVD with settings:
Drive Mode auto
MINLENGTH 55
MAXLENGTH 75
RIPMETHOD mkv
MAINFEATURE False

  • Docker

Ubuntu 22.04.5
A.R.M version: 2.10.5
Current git version: 1ce96f3

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have tested that my fix is effective or that my feature works

Changelog:

  • Add --minlength argument to makemkvcon command in process_single_tracks()

Logs

Schoolhouse_Rock_Disc2_173899117509.log

@sonarqubecloud SonarQubeCloud
Copy link
sonarqubecloud bot commented Feb 8, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@mihawk90
Copy link
Contributor
mihawk90 commented Feb 8, 2025

I'm not sure this is the correct fix for this, see my comment in the bug report.

@microtechno9000 microtechno9000 mentioned this pull request Feb 9, 2025
Open
4 tasks
@mihawk90
Copy link
Contributor
mihawk90 commented Feb 9, 2025

After further discussion this seems to be correct and currently the only way.

@joelishness
Copy link
Contributor Author

Is there anything else needed on this before it can be merged?

@microtechno9000
Copy link
Collaborator

sorry been out sick over the past few weeks

Initial look seems ok, will review and get back to you soon

@microtechno9000
Copy link
Collaborator

Thanks for contributing to ARM, please address the comment against the modified code.

Reach out if there are any issues

@microtechno9000 microtechno9000 added the Awaiting feedback Waiting for user to test fixes label Mar 20, 2025
@joelishness
Copy link
Contributor Author

Thanks for contributing to ARM, please address the comment against the modified code.

Reach out if there are any issues

Is this request directed at me?
If so, sorry but I am not sure what is needed. To which comment are you referring, please?

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@joelishness
Copy link
Contributor Author

Just checking in, anything I can do to help move this along?

@microtechno9000
Copy link
Collaborator

Sorry didnt see your earlier message

I made a comment against the code changed. There is an issue with the implementation for some jobs, when using manual mode.

@joelishness
Copy link
Contributor Author

Sorry didnt see your earlier message

I made a comment against the code changed. There is an issue with the implementation for some jobs, when using manual mode.

Okay, thanks for explaining.
Sorry but I can't see it, and I don't know how to proceed.
If you have some time could you please teach me?

Here is what I see:
Screenshot_20250421_191150
"Show comments" is checked, but I don't see anything that resembles a comment.
Am I looking in the wrong place?

@microtechno9000
Copy link
Collaborator
microtechno9000 commented Apr 22, 2025 •
edited
Loading

Sorry didnt see your earlier message
I made a comment against the code changed. There is an issue with the implementation for some jobs, when using manual mode.

Okay, thanks for explaining. Sorry but I can't see it, and I don't know how to proceed. If you have some time could you please teach me?

Here is what I see: Screenshot_20250421_191150 "Show comments" is checked, but I don't see anything that resembles a comment. Am I looking in the wrong place?

Hey
Thanks for being patient, it is a bit of a learning curve and seems GitHub is not playing nice.

I have added a comment against the code, which for me appears in the conversation/log here
image

The comment is against the code, see below
image

From my review, the issue is that when a user manually selects a track, we don't want the min and max values taken into account otherwise it will end up in a state where the user has selected something that through some hidden config/setting will prevent them from achieving that.

If you can find a way that we can have the manual job still activate, yet also have the min/max settings held, modify the code to facilitate that.

Reach out if you have any issues

@joelishness
Copy link
Contributor Author
8000

From my review, the issue is that when a user manually selects a track, we don't want the min and max values taken into account otherwise it will end up in a state where the user has selected something that through some hidden config/setting will prevent them from achieving that.

I can see this perspective, however my experience was that the opposite problem occurred:
The user is presented with Tracks that cannot be ripped (<120 seconds), and trying to do so results in fatal error.
See Issue #1301

If you can find a way that we can have the manual job still activate, yet also have the min/max settings held, modify the code to facilitate that.

Sorry but this request is a bit out of my skill range.
With this pull request I was simply trying to resolve a regression that was introduced with Manual mode.
I am not up to the task of changing or further enhancing the Manual mode.
I humbly suggest that instead the Manual Mode author(s) should revisit this topic to resolve the regression.

tjdavey
tjdavey reviewed May 16, 2025
View reviewed changes
microtechno9000
microtechno9000 requested changes Jun 9, 2025
View reviewed changes
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@joelishness
Copy link
Contributor Author

Summary

There is no demerit to this patch.

Regarding the primary concern (copied from comment still pending):

The only issue with adding this back in, is that when called from a Manual job Line #91, the job will use the min length field, which if a user has manually asked for should ignore and just get the whole thing even if it is smaller than the min length

Please amend to facilitate this

Response 1 to this concern:
My testing today shows that this behavior is true regardless of whether this patch is applied or not.
Not sure if something changed since the Issue and Pull request were submitted, but even in the main A.R.M version 2.15.1 (git version: 91a5404) the user is not presented with tracks that are less than minlength.
I don't believe there is any basis for this concern. See details below.

Response 2 to this concern:
Even if the user specifies an appropriate minlength and is presented with tracks as expected in Manual, without this patch the user cannot get explicitly selected tracks that are <120sec. See details below.
This is the crux of the issue this Pull request aims to fix (for both Auto and Manual modes).

Please review carefully because I may have made a mistake. But as far as I can tell there is simply no reason to hold back this patch and continue with the current broken implementation.

I agree that there is further room for improvement regarding Manual mode, but it is outside the scope of this Pull request which is for a quick and simple regression fix.

Regarding the if/else proposal from @tjdavey:

I tested this today, but unfortunately a rip does not start at all.
I suspect a syntax issue, but I haven't spent any time trying to debug it.
I don't think it is necessary, considering Response 1 above.

Testing details

Default

docker exec -it arm-rippers grep -A 1 "f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)}" /opt/arm/arm/ripper/makemkv.py

          f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)} ' \
          f'--minlength={job.config.MINLENGTH}'
--
                  f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)}'
            run_makemkv(cmd, logfile)

STAR_TREK_GENERATIONS.log
Min 600
Max 4200
Manual

Track # Length (sec) FPS Aspect Ratio Main Feature Manual Process Ripped
0 7078 23.976 16:9 False True
1 1540 29.97 4:3 False True
2 1362 29.97 4:3 False True
3 644 29.97 4:3 False True
4 1178 29.97 4:3 False True
5 769 29.97 4:3 False True
6 828 29.97 4:3 False True
7 621 23.976 16:9 False True
8 743 23.976 16:9 False True
9 668 29.97 4:3 False True
10 831 29.97 4:3 False True
11 1990 29.97 4:3 False True

Result: tracks less than minlength are not presented (same as with Pull request patch)
Invalidates concern with Pull request patch

STAR_TREK_GENERATIONS_174951004361.log
Min 60
Max 4200
Manual

Track # Length (sec) FPS Aspect Ratio Main Feature Manual Process Ripped
0 7078 23.976 16:9 False True
1 1540 29.97 4:3 False True
2 459 23.976 16:9 False True
3 1362 29.97 4:3 False True
4 537 23.976 16:9 False True
5 579 29.97 4:3 False True
6 644 29.97 4:3 False True
7 212 29.97 4:3 False True
8 428 29.97 4:3 False True
9 290 29.97 4:3 False True
10 1178 29.97 4:3 False True
11 769 29.97 4:3 False True
12 425 29.97 4:3 False True
13 828 29.97 4:3 False True
14 304 23.976 16:9 False True
15 563 29.97 4:3 False True
16 621 23.976 16:9 False True
17 743 23.976 16:9 False True
18 186 23.976 16:9 False True
19 348 29.97 4:3 False True
20 142 29.97 4:3 False True
21 668 29.97 4:3 False True
22 831 29.97 4:3 False True
23 90 23.976 16:9 False [x] True
24 142 23.976 16:9 False True
25 1990 29.97 4:3 False True

Result: Additional tracks are presented (same as with Pull request patch)
Invalidates concern with Pull request patch

Selected only track 23 (90 sec)

Result: wrong track ripped
2:22 = 142 sec = track 24
Confirms Issue #1301 problem

STAR_TREK_GENERATIONS_174951096112.log
Min 60
Max 100
Auto

Result: wrong track ripped (exceeds maxlength)
2:22 = 142 sec = track 24
Confirms Issue #1299 problem

if else

docker exec -it arm-rippers grep -A 1 "f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)}" /opt/arm/arm/ripper/makemkv.py

          f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)} ' \
          f'--minlength={job.config.MINLENGTH}'
--
                  f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)} ' \
                  f'--minlength={job.config.MINLENGTH if mode == 'auto' else 0}'

Nothing happens in the web GUI after inserting disc

docker logs arm-rippers

Jun 10 14:40:23 44b557ec0838 ARM: [ARM] Entering docker wrapper  
Jun 10 14:40:23 44b557ec0838 ARM: Tue Jun 10 14:40:23 UTC 2025 [[ARM] Starting ARM for Bluray on sr0

Original Pull request

docker exec -it arm-rippers grep -A 1 "f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)}" /opt/arm/arm/ripper/makemkv.py

          f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)} ' \
          f'--minlength={job.config.MINLENGTH}'
--
                  f'dev:{job.devpath} {track.track_number} {shlex.quote(rawpath)} ' \
                  f'--minlength={job.config.MINLENGTH}'

STAR_TREK_GENERATIONS_174956691969.log
Min 600
Max 4200
Manual

Track # Length (sec) FPS Aspect Ratio Main Feature Manual Process Ripped
0 7078 23.976 16:9 False True
1 1540 29.97 4:3 False True
2 1362 29.97 4:3 False True
3 644 29.97 4:3 False True
4 1178 29.97 4:3 False True
5 769 29.97 4:3 False Tru 67E6 e
6 828 29.97 4:3 False True
7 621 23.976 16:9 False True
8 743 23.976 16:9 False True
9 668 29.97 4:3 False True
10 831 29.97 4:3 False True
11 1990 29.97 4:3 False True

Result: tracks less than minlength are not presented (same as without Pull request patch)
Invalidates concern against Pull request patch

STAR_TREK_GENERATIONS_174956712310.log
Min 60
Max 4200
Manual

Track # Length (sec) FPS Aspect Ratio Main Feature Manual Process Ripped
0 7078 23.976 16:9 False True
1 1540 29.97 4:3 False True
2 459 23.976 16:9 False True
3 1362 29.97 4:3 False True
4 537 23.976 16:9 False True
5 579 29.97 4:3 False True
6 644 29.97 4:3 False True
7 212 29.97 4:3 False True
8 428 29.97 4:3 False True
9 290 29.97 4:3 False True
10 1178 29.97 4:3 False True
11 769 29.97 4:3 False True
12 425 29.97 4:3 False True
13 828 29.97 4:3 False True
14 304 23.976 16:9 False True
15 563 29.97 4:3 False True
16 621 23.976 16:9 False True
17 743 23.976 16:9 False True
18 186 23.976 16:9 False True
19 348 29.97 4:3 False True
20 142 29.97 4:3 False True
21 668 29.97 4:3 False True
22 831 29.97 4:3 False True
23 90 23.976 16:9 False [x] True
24 142 23.976 16:9 False True
25 1990 29.97 4:3 False True

Result: Additional tracks are presented (same as without Pull request patch)
Invalidates concern against Pull request patch

Selected only track 23 (90 sec)

Result: correct track ripped
1:30 = 90 sec = track 23
Confirms fix for Issue #1301

STAR_TREK_GENERATIONS_174956771199.log
Min 60
Max 100
Auto

Result: correct track ripped
1:30 = 90 sec = track 23
Confirms fix for Issue #1299

@joelishness
Copy link
Contributor Author

Hello,
Just checking in on this, anything we can do to make progress?

To recap my previous comment, I do not believe there are any problems with this PR.
I agree that there is further room for improvement regarding Manual mode, but it is outside the scope of this Pull request which is simply for a quick regression fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@microtechno9000 microtechno9000 microtechno9000 requested changes

+1 more reviewer

@tjdavey tjdavey tjdavey left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Awaiting feedback Waiting for user to test fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🐛 MINLENGTH not respected, always 120
4 participants
@joelishness @mihawk90 @microtechno9000 @tjdavey
0