cropdetect values cut too much

[JouMxyzptlk]

Summary of the bug:

cropdetect uses modulo for its second parameter, possibly cutting of parts of a video instead of preserving the video with less black bars.

How to reproduce:

Testvideo: ​http://joumxyzptlk.de/tmp/ffplay-autoexit-not-working.mkv
Actual crop values would be w=1479:h=1079:x=220:y=0 (or codec freindly w=1480:h=1080:x=220:y=0)

C:\tmp>c:\prog\ffmpeg\bin\ffmpeg -i ffplay-autoexit-not-working.mkv -frames:v 2 -vf cropdetect -f null NUL
ffmpeg version N-93139-ga9452fe6dc Copyright (c) 2000-2019 the FFmpeg developers
  built with gcc 8.2.1 (GCC) 20190212
  configuration: --enable-gpl --enable-version3 --enable-sdl2 --enable-fontconfig --enable-gnutls --enable-iconv --enable-libass --enable-libdav1d --enable-libbluray --enable-libfreetype --enable-libmp3lame --enable-libopencore-amrnb --enable-libopencore-amrwb --enable-libopenjpeg --enable-libopus --enable-libshine --enable-libsnappy --enable-libsoxr --enable-libtheora --enable-libtwolame --enable-libvpx --enable-libwavpack --enable-libwebp --enable-libx264 --enable-libx265 --enable-libxml2 --enable-libzimg --enable-lzma --enable-zlib --enable-gmp --enable-libvidstab --enable-libvorbis --enable-libvo-amrwbenc --enable-libmysofa --enable-libspeex --enable-libxvid --enable-libaom --enable-libmfx --enable-amf --enable-ffnvcodec --enable-cuvid --enable-d3d11va --enable-nvenc --enable-nvdec --enable-dxva2 --enable-avisynth --enable-libopenmpt
  libavutil      56. 26.100 / 56. 26.100
  libavcodec     58. 47.102 / 58. 47.102
  libavformat    58. 26.101 / 58. 26.101
  libavdevice    58.  6.101 / 58.  6.101
  libavfilter     7. 48.100 /  7. 48.100
  libswscale      5.  4.100 /  5.  4.100
  libswresample   3.  4.100 /  3.  4.100
  libpostproc    55.  4.100 / 55.  4.100
Input #0, matroska,webm, from 'ffplay-autoexit-not-working.mkv':
  Metadata:
    ENCODER         : Lavf57.71.100
  Duration: 00:00:24.97, start: 0.020000, bitrate: 3079 kb/s
    Stream #0:0: Video: h264 (High), yuv420p(progressive), 1920x1080 [SAR 1:1 DAR 16:9], 24.39 fps, 24.39 tbr, 1k tbn, 47.95 tbc (default)
    Metadata:
      DURATION        : 00:00:24.966000000
    Stream #0:1(jpn): Audio: aac (LC), 48000 Hz, stereo, fltp (default)
    Metadata:
      DURATION        : 00:00:24.681000000
Stream mapping:
  Stream #0:0 -> #0:0 (h264 (native) -> wrapped_avframe (native))
  Stream #0:1 -> #0:1 (aac (native) -> pcm_s16le (native))
Press [q] to stop, [?] for help
Output #0, null, to 'NUL':
  Metadata:
    encoder         : Lavf58.26.101
    Stream #0:0: Video: wrapped_avframe, yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 24.39 fps, 24.39 tbn, 24.39 tbc (default)
    Metadata:
      DURATION        : 00:00:24.966000000
      encoder         : Lavc58.47.102 wrapped_avframe
    Stream #0:1(jpn): Audio: pcm_s16le, 48000 Hz, stereo, s16, 1536 kb/s (default)
    Metadata:
      DURATION        : 00:00:24.681000000
      encoder         : Lavc58.47.102 pcm_s16le
[Parsed_cropdetect_0 @ 000001dd37f3a900] x1:220 x2:1698 y1:0 y2:1078 w:1472 h:1072 x:224 y:4 pts:381 t:0.381000 crop=1472:1072:224:4
frame=    2 fps=0.0 q=-0.0 Lsize=N/A time=00:00:00.74 bitrate=N/A speed=22.3x
video:1kB audio:140kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown

Detected crop values are: w=1472:h=1072:x=224:y=4
Expected crop values are: w=1480:h=1080:x=220:y=0

Even when using -vf "cropdetect=24:2:0" I get w=1478:h=1078:x=220:y=0 - but at least X and Y are sane values when using modulo 2.
-vf "cropdetect=24:128:0" results in w=1408:h=1024:x=256:y=28

Handbrake suffers from the same issue, so fixing it in ffmpeg might fix it in the next Handbrake version automatically...

We need an option to prefer smaller black borders over killing parts of the video.
The suggested fix to keep compatibility: A fourth parameter in cropdetect, set to "1" and it prefers keeping small black bars instead of clipping the actual content.
My current workaround for cropdetect=24:2:0 is to add 2 to width and height of the detected crop values before using them in the subsequent ffmpeg encoding command.

[JouMxyzptlk]

Current workaround for my encoding batch, calculating w= and h= manually since the detection of the x1 / x2 / y1 / y2 are correct:

set modulo=2
c:\prog\ffmpeg\bin\ffmpeg -hide_banner -ss 00:05:00 -loglevel info -i %input% -frames:v 2000 -vf "cropdetect=16:%modulo%:2000" -f null NUL 2>&1 | grep -o -e "[0-9]\{1,\} x2:.*y2:[0-9]\{1,\}" | tail -1 | sed -e s/" [a-z][0-9]:"/" "/g > %temp%\crop.txt
set a= && set b= && set c= && set d=
for /f "tokens=1-4" %%a in (%temp%\crop.txt) do set x1=%%a&& set x2=%%b&& set y1=%%c&& set y2=%%d
set /a w=%x2%+1-%x1%
set /a h=%y2%+1-%y1%
:wcheck
set /a w2=%w%/%modulo%*%modulo%
if not "%w2%"=="%w%" set /a w=%w%+1 && goto :wcheck
:hcheck
set /a h2=%h%/%modulo%*%modulo%
if not "%h2%"=="%h%" set /a h=%h%+1 && goto :hcheck
set crop=crop=%w%:%h%:%x1%:%y1%
echo %crop%

You shouldn't use modulo higher that 8 since 1080 / 16 is 67.5, ending up with h=1088 when using this calc method.

[Carl Eugen Hoyos]

Replying to JouMxyzptlk:

We need an option to prefer smaller black borders over killing parts of the video.

This is not how cropdetect is supposed to work...

[JouMxyzptlk]

Replying to cehoyos:

Replying to JouMxyzptlk:

We need an option to prefer smaller black borders over killing parts of the video.

This is not how cropdetect is supposed to work...

There is no documentation anywhere on "supposed to work", so it is not defined. It simply got implemented the destructive way. The "supposed" discussion though is nothing for a bug report, that is something for a forum where we can slap opinions around which way is supposed to be the only right way.

I miss having the ability to choose whether to sacrifice actual content being cut away, or to live with the smallest black bars possible within the target codec constraints. Therefore my enhancement suggestion to give us the choice.

Using my .CMD I simply worked around that issue so I can blindly trust the result being non-destructive. But a ffmpeg-internal implementation is, IMHO, better since a lot of other projects use ffmpeg routines and will benefit.

[Carl Eugen Hoyos]

Replying to JouMxyzptlk:

Replying to cehoyos:

Replying to JouMxyzptlk:

We need an option to prefer smaller black borders over killing parts of the video.

This is not how cropdetect is supposed to work...

There is no documentation anywhere on "supposed to work"

No.
There are only two decades of using features similar to our cropdetect filter in different transcoding applications.
Or two explain it differently: What you suggest, completely (!) ruins the effect the filter is supposed to have since forever. This was never about a beautification of the output video but about removing an artefact that makes encoding significantly more difficult leading to either worse output or bigger files.
I am not opposed to allow the cropdetect filter to change its behaviour (and I expect it is technically possible) but it has to be made clear that what you describe is not a bug and what you expect would be considered a bug if done by default.