[BUGFIX] Fix MINLENGTH for track.process by joelishness · Pull Request #1302 · automatic-ripping-machine/automatic-ripping-machine

joelishness · 2025-02-08T05:35:33Z

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 · 2025-02-08T05:36:05Z

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 · 2025-02-08T06:39:21Z

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

mihawk90 · 2025-02-09T16:43:39Z

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

joelishness · 2025-02-25T23:36:58Z

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

microtechno9000 · 2025-02-26T00:09:20Z

sorry been out sick over the past few weeks

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

microtechno9000 · 2025-03-20T12:44:55Z

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

Reach out if there are any issues

joelishness · 2025-03-28T17:58:02Z

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 · 2025-03-30T16:23:43Z

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 · 2025-04-12T03:49:01Z

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

microtechno9000 · 2025-04-12T04:55:27Z

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 · 2025-04-22T02:14:22Z

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:

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

microtechno9000 · 2025-04-22T13:37:23Z

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: "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

The comment is against the code, see below

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 · 2025-05-05T14:23:57Z

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.

arm/ripper/makemkv.py

sonarqubecloud · 2025-06-10T15:50:12Z

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 · 2025-06-10T16:00:34Z

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	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	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 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 · 2025-07-21T16:43:50Z

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.

1337-server · 2025-10-26T06:19:53Z

This is now out of date with the codebase, please update.

This does two things: 1. Enables listing of tracks <120s for 'manual mode' (otherwise track listing only shows >120s per makemkv defaults) 2. Adds MINLENGTH to track ripping (otherwise tracks <120s fail to rip per makemkv defaults)

microtechno9000 · 2026-02-04T12:09:57Z

From your testing it looks like the patch is resolving the assumed/expected behavior. Agree manual issues are outside the scope of the PR, but the initial commits appeared to break it. Although your testing proved otherwise, cheers for that.

microtechno9000

LGTM

sonarqubecloud · 2026-02-04T12:12:10Z

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

microtechno9000 mentioned this pull request Feb 9, 2025

🐛 MINLENGTH not respected, always 120 #1299

Closed

4 tasks

microtechno9000 added the Awaiting feedback Waiting for user to test fixes label Mar 20, 2025

tjdavey reviewed May 16, 2025

View reviewed changes

arm/ripper/makemkv.py Outdated Show resolved Hide resolved

microtechno9000 requested changes Jun 9, 2025

View reviewed changes

arm/ripper/makemkv.py Outdated Show resolved Hide resolved

arm/ripper/makemkv.py Outdated Show resolved Hide resolved

1337-server added the Stale Issue hasnt had a response in a long time label Oct 26, 2025

joelishness closed this Dec 29, 2025

joelishness force-pushed the patch-1 branch from 05a3ff5 to 5132031 Compare December 29, 2025 18:34

Add --minlength option to MakeMKV commands

72297f6

This does two things: 1. Enables listing of tracks <120s for 'manual mode' (otherwise track listing only shows >120s per makemkv defaults) 2. Adds MINLENGTH to track ripping (otherwise tracks <120s fail to rip per makemkv defaults)

joelishness reopened this Dec 29, 2025

joelishness added 3 commits December 29, 2025 11:53

Removed blank line

90cd106

Fix missing cmd line

b575527

Fix extraneous f-string without placeholder

282ec47

joelishness requested a review from microtechno9000 December 29, 2025 20:14

Merge branch 'main' into patch-1

cb71c16

microtechno9000 removed the Stale Issue hasnt had a response in a long time label Feb 4, 2026

microtechno9000 added 2 commits February 4, 2026 22:40

Merge branch 'main' into patch-1

2a23efb

Bump version from 2.21.2 to 2.21.3

6c46b4e

microtechno9000 approved these changes Feb 4, 2026

View reviewed changes

microtechno9000 merged commit 6716ad7 into automatic-ripping-machine:main Feb 4, 2026
11 of 12 checks passed

joelishness mentioned this pull request Feb 13, 2026

🐛 Drive Mode Manual does not rip the correct track #1301

Closed

4 tasks

Conversation

joelishness commented Feb 8, 2025

Description

Type of change

How Has This Been Tested?

Checklist:

Changelog:

Logs

Uh oh!

sonarqubecloud bot commented Feb 8, 2025

Quality Gate passed

Uh oh!

mihawk90 commented Feb 8, 2025

Uh oh!

mihawk90 commented Feb 9, 2025

Uh oh!

joelishness commented Feb 25, 2025

Uh oh!

microtechno9000 commented Feb 26, 2025

Uh oh!

microtechno9000 commented Mar 20, 2025

Uh oh!

joelishness commented Mar 28, 2025

Uh oh!

sonarqubecloud bot commented Mar 30, 2025

Quality Gate passed

Uh oh!

joelishness commented Apr 12, 2025

Uh oh!

microtechno9000 commented Apr 12, 2025

Uh oh!

joelishness commented Apr 22, 2025

Uh oh!

microtechno9000 commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelishness commented May 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Jun 10, 2025

Quality Gate passed

Uh oh!

joelishness commented Jun 10, 2025

Summary

Testing details

Default

if else

Original Pull request

Uh oh!

joelishness commented Jul 21, 2025

Uh oh!

1337-server commented Oct 26, 2025

Uh oh!

microtechno9000 commented Feb 4, 2026

Uh oh!

microtechno9000 left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 4, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

microtechno9000 commented Apr 22, 2025 •

edited

Loading