Proposal to no longer constrain detected crop results

Question

Proposal to no longer constrain detected crop results

donmelton opened this issue 8 years ago · 20 comments

Proposal to no longer constrain detected crop results

In the past, I've been adamant that cropping video for transcoding is not about making the edges of the output look pretty. My rationale was that those "black" edges are not visible anyway when viewed full screen. For me, cropping was about improving performance, not aesthetics.

So, I designed the detect-crop tool and the --crop detect function of transcode-video to determine what I believed to be the "optimal" shape for a crop. The algorithm for this shape would only crop a minimum number of pixels, and always from either the top and bottom or from the left and right, never from both axes.

But I now believe it's a much better practice to remove all the "black" edges around the video when transcoding. Of course, there are a few exceptions for irregularly cropped movies like "The Dark Knight (2008)" and "The Grand Budapest Hotel (2014)."

Why did I change my mind? After transcoding thousands of videos, I can identify more than just a few where leaving "black" edges causes annoying artifacts as x264 tries to encode the actual boundary of the content next to the remaining edge. This can be both noticeable and distracting, lowering the perceived quality of the output.

Currently, both the detect-crop tool and the --crop detect function of transcode-video can be configured to ignore the "optimal" shape algorithm. This is done via the --no-constrain and --no-constrain-crop options. So, I propose to make that the default behavior. However, I would still allow access to that "optimal" shape algorithm via new --constrain and --constrain-crop options.

The --help output of detect-crop would change to:

    --constrain     constrain crop to optimal shape

And the --help output of transcode-video would change to:

    --crop T:B:L:R  set video crop values (default: 0:0:0:0)
                      (use `--crop detect` for `detect-crop` behavior)
                      (use `--crop auto` for `HandBrakeCLI` behavior)
    --constrain-crop
                    constrain `--crop detect` to optimal shape

Of course, the old --no-constrain and --no-constrain-crop options would continue to be allowed, undocumented, until the next major revision. But they wouldn't actually do anything since that would be the default behavior anyway.

So, I'm asking for feedback. Let me know what you think. Soon. Because I'm liable to implement this as early as next week since the change is trivial due to the way I've architected the code. Basically, it's just flipping a boolean and changing the documentation. The hardest work will be re-writing the README document.

Thanks.

Answer 1 · 2016-09-11T18:26:41.000Z

BTW, there is a downside to changing the default behavior. Unambiguous crop values are less likely since any disagreements between HandBrakeCLI and mplayer will no longer be obscured by the "optimal" shape algorithm.

Answer 2 · 2016-09-12T02:24:46.000Z

Well, I always transcoded thismway and was very happy and weclome this change.

Does this mean that the detect-crop auto behavior option of transcode-video would also reflect that change?

Answer 3 · 2016-09-12T02:46:06.000Z

@JMoVS Yes, the --crop detect option in transcode-video would also no longer constrain to the optimal shape by default. And you would have to use a new --constrain-crop option to get the old behavior. Hopefully that's exactly what you want.

Answer 4 · 2016-09-12T03:50:28.000Z

Yes indeed, I want to always use the unconstrained crop. So this sounds like a very welcome change to me! 👍

Answer 5 · 2016-09-12T05:46:53.000Z

As one of your less technical and, as a result, possibly even more grateful, followers, is it possible to automatically chose the most conservative crop option in the cases where one runs into differing results from the two programs?

That's what I've been doing manually whenever there are differing crop values.

No matter what you choose to implement, thanks again for doing the heavy lifting.

Best,
Plato

Get Outlook for iOShttps://aka.ms/o0ukef

From: JMoVS <notifications@github.com mailto:notifications@github.com>
Sent: Sunday, September 11, 2016 8:50 PM
Subject: Re: [donmelton/video_transcoding] Proposal to no longer constrain detected crop results (#81)
To: donmelton/video_transcoding <video_transcoding@noreply.github.com mailto:video_transcoding@noreply.github.com>

Yes indeed, I want to always use the unconstrained crop. So this sounds like a very welcome change to me! ?

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com//issues/81#issuecomment-246237600, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AOepbe4LaxaPVW66pMdr5GC0P4t3Re8Jks5qpMwGgaJpZM4J6EMf.

Answer 6 · 2016-09-12T06:24:18.000Z

Fully agree, I also think it is better to always remove all the black lines from the video. In the end, the black lines are no content, an "implementation detail", the encoder should not be bothered with.

Answer 7 · 2016-09-12T12:07:01.000Z

Platonium: this can sometimes still lead to more being cropped than should be. I had movies where both mplayer and handbrake cropped too much, just in different directions. So I guess this is something that should at least be discussed on how this is handled.

Answer 8 · 2016-09-12T14:46:46.000Z

@platonium The problem with automatically picking the most conservative crop between the two choices provided by HandBrakeCLI and mplayer is that sometimes mplayer returns a result of 0:0:0:0, or no crop at all. And it's impossible to tell programmatically whether it's being prudent, as is the case with "The Dark Knight (2008)," or just being stupid, as is the case with dozens of other movies.

That's why I never use any of the automatic cropping behaviors myself, even though I provide them for my users. I always manually review the choices provided by HandBrakeCLI and mplayer before I transcode. I've learned over the years that you just can't trust either one of them all the time.

And, BTW, you are very welcome! It's my pleasure to do the heavy lifting. I'm glad the software is working for you.

Answer 9 · 2016-09-12T14:49:18.000Z

@t089 Glad to hear it. Thanks. Yeah, I suspected there were many users like yourself and @JMoVS who were, it turns out, smartly ignoring my orginal advice and cropping away the black edges. :)

Answer 10 · 2016-09-13T01:40:26.000Z

I've been using --no-constrain-crop every time I use detect-crop, so Im find with it being the default.

Answer 11 · 2016-09-13T03:01:02.000Z

@cameronks Cool. That's yet another vote for the change. Thanks! :)

Answer 12 · 2016-09-15T19:45:40.000Z

I crop away all black edges also.

Answer 13 · 2016-09-15T19:55:49.000Z

@gingerbeardman Great! Thanks for the feedback. This is starting to look unanimous. :)

Answer 14 · 2016-09-15T20:06:19.000Z

The only other thing I try to do is maintain the correct aspect ratio.

Answer 15 · 2016-09-15T20:08:52.000Z

@gingerbeardman Maintaining aspect ratio shouldn't be a problem as long as you're not using the --720p option or otherwise changing the size of the output.

Answer 16 · 2016-09-15T22:23:55.000Z

This is a welcome change from my perspective. Thumbs up!

Answer 17 · 2016-09-15T23:13:42.000Z

@RodBrown1988 Thanks and you are very welcome! The code is done and now I'm just writing the documentation. I'll probably release it tonight.

Answer 18 · 2016-09-16T01:02:39.000Z

Makes sense. Thanks for all your work and thanks for asking.

Answer 19 · 2016-09-16T01:33:46.000Z

@dennya You are very welcome, sir!

Answer 20 · 2016-09-16T01:44:20.000Z

OK, folks. I released version 0.11.0 with this change. Just gem update to get it.

Thank you all!