fallback-back crop auto option request

[dkoenig01]

I propose an option on the fallback-crop setting that would select the most conservative values from either the Handbrake or ffmpeg results, thus providing a hedge between the current behavior of trusting either result exclusively or the none option.

[lisamelton]

@dkoenig01 Interesting! Thanks for opening this issue.

OK, by "conservative" do you mean the least amount of cropping? And do you envision this as a new argument to the --fallback-crop option?

[lisamelton]

@dkoenig01 D'oh! Of course you mean it's an argument to the --fallback-crop option because you have that in the title.

My apologies for being such a dunderhead. :)

[lisamelton]

@dkoenig01 OK, since this shouldn't be too hard to implement (I hope), I'm working on a --help description now. Stay tuned...

[lisamelton]

@dkoenig01 Here's my first draft of a --help description:

    --fallback-crop handbrake|ffmpeg|minimal|none
                    select fallback crop values if `--crop detect` fails
                      (`minimal` uses the smallest possible crop values
                        combining results from `HandBrakeCLI` and `ffmpeg`)

How does that sound?

[lisamelton]

@dkoenig01 Also, I'm honored that this is your first-ever issue on GitHub. Congratulations!

[dkoenig01]

Excellent, that looks perfect! While someone may have use for the higher amount of cropping to avoid any letter boxing/pillarboxing, I know I'd prefer the lower value value which would be least likely to overcrop the image. Perhaps providing both options of "minimal" and "maximal" should anyone desiree the alternative? Thanks!

[lisamelton]

@dkoenig01 Interesting! I will also consider "maximal" although, as you very correctly identified, it's a much riskier strategy.

And you are very welcome!

Hopefully I can get some feedback on this issue and proposal from the usual suspects as well, like @samhutchins, @JMoVS, @ttyS0, @khaosx, @RodBrown1988, @vr8hub, @klogg416 and others. Bueller? Bueller? Bueller?

[dkoenig01]

I've avoided the community too long! Thanks for providing such a helpful solution to churn out my Plex library.

[RodBrown1988]

Definite +1 from me.

Seems a no-brainer to fallback to lesser of the two. Better to minimise the cropping than risk cropping the picture.

[lisamelton]

@dkoenig01 No problem! Provided this change I'm hacking on now actually works, I can probably get this in the next release sometime during the next two weeks.

[lisamelton]

@RodBrown1988 Agreed! And thanks for the feedback, sir.

[lisamelton]

And here's a working patch for the whole feature:

diff --git a/bin/transcode-video b/bin/transcode-video
index ac67a36..7b951be 100755
--- a/bin/transcode-video
+++ b/bin/transcode-video
@@ -89,8 +89,10 @@ Video options:
                       (use `--crop auto` for `HandBrakeCLI` behavior)
     --constrain-crop
                     constrain `--crop detect` to optimal shape
-    --fallback-crop handbrake|ffmpeg|none
+    --fallback-crop handbrake|ffmpeg|minimal|none
                     select fallback crop values if `--crop detect` fails
+                      (`minimal` uses the smallest possible crop values
+                        combining results from `HandBrakeCLI` and `ffmpeg`)
     --720p          fit video within 1280x720 pixel bounds
     --max-width WIDTH, --max-height HEIGHT
                     fit video within horizontal and/or vertical pixel bounds
@@ -430,7 +432,7 @@ HERE

       opts.on '--fallback-crop ARG' do |arg|
         @fallback_crop = case arg
-        when 'handbrake', 'ffmpeg', 'mplayer', 'none'
+        when 'handbrake', 'ffmpeg', 'mplayer', 'minimal', 'none'
           arg.to_sym
         else
           fail UsageError, "invalid fallback crop argument: #{arg}"
@@ -1053,6 +1055,13 @@ HERE
               hb_crop
             when :ffmpeg, :mplayer
               ff_crop
+            when :minimal
+              {
+                :top    => [hb_crop[:top],    ff_crop[:top]].min,
+                :bottom => [hb_crop[:bottom], ff_crop[:bottom]].min,
+                :left   => [hb_crop[:left],   ff_crop[:left]].min,
+                :right  => [hb_crop[:right],  ff_crop[:right]].min
+              }
             when :none
               {:top => 0, :bottom => 0, :left => 0, :right => 0}
             else

Honestly @dkoenig01, I can't believe I hadn't thought of this before.

And hopefully no one else has suggested this and I stupidly ignored them!

[lisamelton]

@dkoenig01 BTW, I did some more testing and most of the time minimal returns the same results as choosing ffmpeg. But not always! :)

[dkoenig01]

Thanks. That seems to agree with my limited observations so far, but I didn't think I could count on it always being the case.

[dkoenig01]

Oh and just saw the patch. You're a saint!

[khaosx]

Thumbs up from me also, and a very good idea from @dkoenig01 . I've had good luck with using ffmpeg as the fallback, but I think I'll swap to minimal just to ensure a sanity check.

[lisamelton]

@khaosx Thanks for the quick feedback, sir! Yeah, I suspect quite a few people were using ffmpeg as the fallback now that I think about it.

[vr8hub]

I'm amazed that merely bombarding Don with questions results in inclusion in "the usual suspects," but I'll take what recognition I can get.

Definite thumbs up from me for minimal, but I can't imagine what maximal would be good for. That sounds as useful as a --letterbox option to me. (But we've already established I don't know what I'm talking about. I thought defaulting hardware encoders sounded like a good idea. I was so naive when I was [three days] young[er].)

[lisamelton]

@vr8hub Thank you, also, for the quick feedback, sir! And your instincts on maximal agree with mine and @RodBrown1988 here. No need for apologies.

[lisamelton]

@vr8hub BTW, I'm not giving a definite "no" to @dkoenig01 's question about maximal. It's certainly simple to implement as you can see from the patch. It's just my cautious nature here. :)

[samhutchins]

minimal would be useful in batch-encoding situations, but I don't think maximal is useful

[JMoVS]

Hmm I have the following concern: I had cases where only the minimal combination of handbrake+ffmpeg resulted in best results. So I think it shouldn't be "minimal" of either or handbrake/ffmpeg but instead compare crop value in each of the 4 directions and then use the minimum in all 4 directions

[JMoVS]

And I think for people with small costs of effort and time to make a transcode, the maximal could also be useful.

Though now that I'm thinking of it, would people and dumbheads like me get confused by what a "minimal" and "maximal" crop value means? Is that that clear? Or should we use "conservative" and "aggressive" or sth like that?

[samhutchins]

@JMoVS

Hmm I have the following concern: I had cases where only the minimal combination of handbrake+ffmpeg resulted in best results. So I think it shouldn't be "minimal" of either or handbrake/ffmpeg but instead compare crop value in each of the 4 directions and then use the minimum in all 4 directions

Agreed, and if I'm reading Don's patch correctly, that's exactly what it does :-)

[skj-dev]

This definitely falls into the category of a feature that I didn't realize I really wanted until I saw it. 😁

I don't see me trusting maximal, though. If you decide to include it, perhaps add a small warning in the help description?

[klogg416]

Nothing to add @donmelton, but it is a great idea. The minimal setting automates my first step after recording (running —crop-detect) effectively replacing the need for me to keep the crop values file with each source make.

Maximal would be more fun if it randomly generated the crop area and you get what you get. :-)

On Feb 18, 2019, at 9:20 AM, Sean Johnson notifications@github.com wrote:

This definitely falls into the category of a feature that I didn't realize I really wanted until I saw it. 😁

I don't see me trusting maximal, though. If you decide to include it, perhaps add a small warning in the help description?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[vr8hub]

… but instead compare crop value in each of the 4 directions and then use the minimum in all 4 directions

Yes, see the patch Don posted earlier. That's exactly what he does.

[lisamelton]

@JMoVS @samhutchins @ttyS0 @klogg416 @vr8hub Thanks for all the feedback this morning!

Yep, Sam is correct, that is what the patch does.

And many of you are expressing even more concerns about adding a "maximal" option than I considered. With Kyle's feature extension of it being especially hysterical. :)

[lisamelton]

@dkoenig01 @JMoVS @samhutchins @ttyS0 @klogg416 @vr8hub I'm high on a second cup of coffee and just spitballing here, but in the future should we consider making "minimal" the default behavior for "--crop detect"? Which would mean making the current failure behavior an option argument for "--fallback-crop"? Or am I even more buzzed than I thought and one of you has already suggested this? :)

[skj-dev]

... should we consider making "minimal" the default behavior for "--crop detect"?

Yessssssss. 🙌

[lisamelton]

@ttyS0 I thought you would like that idea, Sean. :)

[dkoenig01]

I’d support that so that the default behavior of —crop-detect doesn’t trigger failures.

I really didn’t think there should be any need for maximal, but sometimes one man’s trash is another man’s treasure.

Just spitballing here, and maybe being greedy, but a couple more thoughts on how to expand this if anyone agrees on the usefulness without overcomplicating things:

Implement a --fallback-crop-threshold x or something of the sort that would trigger a failure instead of fallback if the two crop results differ by more than x in any dimension, with a value of 0 disabling the feature and being default. This way you could specify a maximum difference before triggering a failure that would warrant further investigation for manual input of crop values. Honestly I haven’t observed enough to see significant differences, so I’d leave it to others to weigh in on the usefulness.

Another possibility would be a —crop-pad x (or l:r:t:b) that would reduce each dimension by the selected amount (result limited to >= 0) to allow for padding should there be the desire to provide a safety buffer. I can see using this on a large batch and then if I don’t see some undercropping in the results (assuming any cropping was performed), I’d know the detected crop values were too aggressive. It would allow for an additional safety for those of us too lazy to use Don’s recommended process :)

On Feb 18, 2019, at 11:23 AM, Don Melton notifications@github.com wrote:

@ttyS0 https://github.com/ttyS0 I thought you would like that idea, Sean. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/donmelton/video_transcoding/issues/255#issuecomment-464849801, or mute the thread https://github.com/notifications/unsubscribe-auth/AT5yQ8eG6VMyOvdnlUn8WVS58RPtrstpks5vOv2dgaJpZM4a_4Dk.

[klogg416]

@donmelton I think that’s a great default, more cautious than the current approach while still enabling a good chance of detecting a crop and a smaller file output.

On Mon, Feb 18, 2019, at 2:06 PM, Don Melton wrote:

@dkoenig01 https://github.com/dkoenig01 @JMoVS https://github.com/JMoVS @samhutchins https://github.com/samhutchins @ttyS0 https://github.com/ttyS0 @klogg416 https://github.com/klogg416 @vr8hub https://github.com/vr8hub I'm high on a second cup of coffee and just spitballing here, but in the future should we consider making "minimal" the default behavior for "--crop detect"? Which would mean making the current failure behavior an option argument for "--fallback-crop"? Or am I even more buzzed than I thought and one of you has already suggested this? :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/donmelton/video_transcoding/issues/255#issuecomment-464845797, or mute the thread https://github.com/notifications/unsubscribe-auth/AHAaMMMTftRHrYElgrqUY1tBm_7bLYOTks5vOvnJgaJpZM4a_4Dk.

[lisamelton]

@dkoenig01 Interesting ideas! Especially that threshold concept. Not hard to implement but maybe hard to explain! Although it would probably always be triggered by those few videos from filmmakers who switch between letterbox/pillarbox formats multiple times. (I'm looking at you, Christopher Nolan and Wes Anderson!)

Padding is also not hard to implement but, I suspect, might cause some edging artifacts. This is why I phased out "optimal" crops as a default behavior.

But keep these ideas coming!

Also, I'm sometimes too lazy to use my recommended cropping process. :)

[dkoenig01]

@donmelton good point on edging artifacts with crop-pad. I'll go ahead and -1 on that idea.

[lisamelton]

@dkoenig01 I'll probably land this tomorrow once @samhutchins give me a thumbs up for our big audio features patch. Which means I may be able to release it by this weekend. Fingers crossed.

If you want, go ahead and open a new issue for making your "minimal" the default mode and maybe another one for that threshold idea of yours. I would love to get more feedback on that. And, hey, maybe someone will try to implement it and send us a patch.

But no hurry!

[lisamelton]

@dkoenig01 OK, this is now checked in! Expect a mention in the "History" section of the "README" document when I release it. :)

[lisamelton]

@dkoenig01 Thanks again for this feature idea, sir!

Just gem update to get version 0.24.0 with it baked inside. :)

And thanks to @RodBrown1988, @khaosx, @vr8hub, @samhutchins, @JMoVS, @ttyS0, @klogg416 and everyone else I forgot to mention for all your feedback on this one! I love this community!

[JMoVS]

I just noticed that maybe an addendum to the https://github.com/donmelton/video_transcoding#using-detect-crop section mentioning the new minimal key would also be appreciated for newcomers.

[lisamelton]

@JMoVS Good idea! Feel free to open an issue on that, too.