[Jellyfin] Limit canvas size

[dmitrylyzo]

Changes
Add an option to limit the canvas size to speed up rendering.

List of commits

345d701
4c5f018
bcf4b5f

[TheOneric]

Seperating only the changes related to a single feature is a good first step, thanks!
The commit-series, however, does not yet meet the criteria described before; but this can still be worked on.

Let's begin with what's wrong:

• The first commit adds a basic form of the feature with the default max height of 1600, the second commit bumps the default to 2160 and the fourth commit changes the default to not cap the size. This violates “no temporary changes”.
• The last commit adds “missing” documentation, but documentation should be added together with the feature.
• Commit number 4 changes the interpretation of one of the options. First committing a basic (but correctly working!) state of a new feature and then expanding it in followups can be sensible, but it also changes the default (again: “temporary changes”). Though I don't understand why 1.0 isn't implemented as a noop to begin with.

In summary, I think the commits at hand should be remolded into two or three commits; either one for both self.prescaleTradeoff and softHeightLimit and a different one for hardHeightLimit or one per option.

Assuming JustAMan as the principal author and two commits, the descriptions for the current content could look something like this after rearraging:

Add options to scale canvas relative to video

<Insert explanation of and justification for feature here>

[Vasily <[email protected]>]
  Original implementation together with a hard canvas height limit in
  https://github.com/jellyfin/JavascriptSubtitlesOctopus/commit/345d7010b1d31f933f1b2cc3a17d83a09b5e5c2e
  https://github.com/jellyfin/JavascriptSubtitlesOctopus/commit/bcf4b5fc62140a3ab37ce5e3f6c329c3094cb7d8
[Dmitry Lyzo <[email protected]>]
  Rebased for upstream; split out hardHeightLimit and added docs

Co-Authored-By: Dmitry Lyzo <[email protected]>

---

Add option to limit canvas size

<Insert explanation of and justification for feature here>

[Vasily <[email protected]>]
  Original implementation together with prescaling in
  https://github.com/jellyfin/JavascriptSubtitlesOctopus/commit/345d7010b1d31f933f1b2cc3a17d83a09b5e5c2e
  https://github.com/jellyfin/JavascriptSubtitlesOctopus/commit/4c5f018a07d5c51b089ef21ccf1f81a0355e6c8c
[Dmitry Lyzo <[email protected]>]
  Rebased for upstream; seperated from the prescaling;
  removed default height limit and added docs

Co-Authored-By: Dmitry Lyzo <[email protected]>

---

Now, beyond the form of the commits I also have some remarks and questions about the content, see below.

[TheOneric]

The comment here is not accurate; this also has an effect for prescaleTradeoff > 1.0.

[TheOneric]

The scaling logic has issues with (divisions by) zero and negative prescale values and seems overly repettive to me.
Iiuc, if the prescale factor is (larger/smaller) than 1, it is supposed to apply scaling when the height is (below/above) the "soft limit” but cap the scaled result to the “soft limit” and always cap the height to the hard limit.
Also I don't understand why prescaleTradeoff == 1.0 can't be the noop.

How about this instead? Iinm, this should deal well withzero and negativity and doesn't need the duplication for (larger/smaller) than 1 and have prescaleFactor == 1.0 as a natural noop:

function _computeCanvasSize(width, height) {
    var hardHeightLimit = Math.max(self.hardHeightLimit || height, self.prescaleHeightLimit);
    var scalefactor = self.prescaleFactor <= 0 ? 1.0 : self.prescaleFactor;

    if (height == 0) {
        width = 0;
    } else {
        var sgn = scalefactor < 1 ? -1 : 1;
        var newH = height;
        if (sgn * newH * scalefactor <= sgn * self.prescaleHeightLimit)
            newH *= self.prescaleFactor;
        else if (sgn * newH < sgn * self.prescaleHeightLimit)
            newH = self.prescaleHeightLimit;
        if (newH > hardHeightLimit)      // Added in the second commit
            newH = hardHeightLimit;      // Added in the second commit
        width *= newH / height;
        height = newH;
    }

    return {'width': width, 'height': height};
}

[dmitrylyzo]

In this case, it would probably be better if you port these features.

[TheOneric]

In this case, it would probably be better if you port these features.

Should I push the amended and redistributed commits to this branch (same PR) or would you prefer a new PR being opened for this?
If you want to keep splitting the jellyfin parts into several standalone series, then for smaller series which don't require familiarity with JS and its worker-model (so eg not renderAhead itself), I can maybe do the squashing and splitting if you absolutely do not want to do that. However, each series must only deal with one concise topic at a time and the commits themselves should also not contain a bunch of unrelated changes.

[dmitrylyzo]

Should I push the amended and redistributed commits to this branch (same PR) or would you prefer a new PR being opened for this?

I think "new PR" since it will be entirely your changes - you literally rewrote the feature.

If you want to keep splitting the jellyfin parts into several standalone series, then for smaller series which don't require familiarity with JS and its worker-model (so eg not renderAhead itself), I can maybe do the squashing and splitting if you absolutely do not want to do that.

The main problem why you and me will never agree is that your team want to have the perfect contribution all at once, and I want to preserve commit history and original contributions (or at least not ruin them too much). I also like the idea of keeping the branch as clean as possible, but this doesn't work well in case of porting - you lose details about contributions (commit message doesn't point the exact lines).

As I said, I can squash some minor commits to where the relevant code appeared first time or an error was introduced, but only if the author is the same. I already did this with that Bump height commit.

My porting scheme: original feature, then fixes/improvements/etc., that way I'm sure I won't break something that already exists (in the feature), and I can review the history in case further "improvements" break it.

I treat the whole branch (PR) as a feature. This gives me more flexibility in the content of the branch, focusing only on the result of the merge.

When bisecting, in any case, you need to localize the bad branch (merge commit) by traversing only the parent (master) branch. Then you bisect the branch. I agree, how quickly you find the error will depend on the quality of the branch. But what do you want to bisect in small features like this?
By squashing commits, you remove the details, and you don't even need to look into the branch - merge commit is the final point.

Tbh, I mostly debug the code as is and only use bisecting for a totally unknown codebase.

However, each series must only deal with one concise topic at a time and the commits themselves should also not contain a bunch of unrelated changes.

I totally agree with you in case of a brand-new features by a single author. I tend to open a separate PR for unrelated changes, as I did for the buffer refactor to free up the renderAhead branch.

Current features "live in harmony" with each other, sharing _computeCanvasSize function. Splitting simply won't provide any significant benefit.

I thank you for your detailed posts and help. I see you want the best for the project, but wouldn't it be better to merge the original features first (with or without patches) and then refactor them as you want?

Currently, there are 4 branches:

• Refactor of buffer
• Canvas size limit (hard limit + prescale)
• renderAhead (renderAhead + dropAnimation)
• Smart blend

[TheOneric]

I think "new PR" since it will be entirely your changes - you literally rewrote the feature.

Will do so then in a couple of days or so.

but wouldn't it be better to merge the original features first (with or without patches) and then refactor them as you want?

Not only is this bad for bisecting and introduces (semi-)broken states into the repository and its history; it is also a hindrance for review and will lead to additional unanticipated bugs slipping through. See for example how I found issues with values <= 0 and could simplify the logic in this PR, and found the (pre-existing) signed-overflow/out-of-bounds-write issues in the buffer PR only after they've been split out. Previously I missed them because I was too distracted by having to keep all future changes and known bugs to ignore in mind.
Unlike this PR which only changes 49 lines in total, for renderAhead splitting alone will not be sufficient to enable proper review and at the same time, the size and complexity of it makes it the most likely to still have bugs lurking.
Small, atomic well-documented commits enable proper review and proper bisecting, which reduces the bugs that are merged at all and makes it easier to deal with the remaining bugs which slip through. In the current state of the renderAhead-branch the individual commits are neither atomic, small (within the bounds of what's possible) nor well-documented.

[dmitrylyzo]

Not only is this bad for bisecting and introduces (semi-)broken states into the repository and its history;

When using merge commits, the master branch will stay unbroken.
Actually, it depends on what is considered "broken". You might miss something or even make a bug anyway. But you can fix the bug or improve the code later.

Previously I missed them because I was too distracted by having to keep all future changes and known bugs to ignore in mind.

That's because you are reviewing per commit.
Again, you could fix them later.

[TheOneric]

Superseded by #127