po5 / thumbfast

High-performance on-the-fly thumbnailer script for mpv
Mozilla Public License 2.0
861 stars 35 forks source link

Thumbnail generation failed with incorrect scale #27

Closed dyphire closed 2 years ago

dyphire commented 2 years ago

Unfortunately, I found that this pr https://github.com/po5/thumbfast/pull/19 did not really solve the scaling accuracy issue, and thumbnails still failed on some videos. I tried to get the following output log:

[   6.283][i][thumbfast] [ffmpeg] filter: Padded dimensions cannot be smaller than input dimensions.
[   6.283][i][thumbfast] [ffmpeg] filter: Failed to configure input pad on filter
[   6.283][i][thumbfast] [lavfi] failed to configure the filter graph
[   6.284][i][thumbfast] Disabling filter pad.00 because it has failed.
[   6.285][i][thumbfast] [autoconvert] Converting yuv420p10 -> bgra
[   6.287][i][thumbfast] VO: [lavc] 299x125 bgra
[   6.287][i][thumbfast] [vo/lavc] Opening encoder: raw video [rawvideo]
[   6.287][i][thumbfast] [encode] Opening muxer: image2 sequence [image2]
dyphire commented 2 years ago

Interestingly, when the following patches are applied, the thumbnails will work.

--- a/thumbfast.lua
+++ b/thumbfast.lua
@@ -168,7 +168,7 @@ local function calc_dimensions()
     local scale = mp.get_property_number("display-hidpi-scale", 1)

     if width / height > options.max_width / options.max_height then
-        effective_w = math.floor(options.max_width * scale + 0.5)
+        effective_w = math.floor(options.max_width * scale - 0.5)
         effective_h = math.floor(height / width * effective_w + 0.5)
     else
         effective_h = math.floor(options.max_height * scale + 0.5)
dyphire commented 2 years ago

After testing, this is more to expose a potential issue. When I disable hidpi and set the thumbnail size to 300x300, thumbnail generation also fails on this video.

natural-harmonia-gropius commented 2 years ago

on this video also fails.

video resolution?

dyphire commented 2 years ago

video resolution?

1920x802

christoph-heinrich commented 2 years ago

I didn't test this, but if you replace effective_h = math.floor(height / width * effective_w + 0.5) with effective_h = math.ceil(height / width * effective_w) does it work?

dyphire commented 2 years ago

I didn't test this, but if you replace effective_h = math.floor(height / width * effective_w + 0.5) with effective_h = math.ceil(height / width * effective_w) does it work?

Yes, the effective_h = math.ceil(height / width * effective_w) works for this.

Edit: This does not really solve the issue. When the thumbnail size is adjusted to 240x240, it fails again.

natural-harmonia-gropius commented 2 years ago

Because those lines been removed, the scaler doesn't support scale to odd size, In some cases it can only be a multiple of 4. https://github.com/po5/thumbfast/commit/5bb27e4904e748b23e15b9fb2f744f7eb5fb3176#diff-6fa795ff14b0660affcdf013d87c0dd55c338000c8dffa297c5c0a350e1f713eL188-R17

christoph-heinrich commented 2 years ago

the scaler doesn't support scale to odd size

How do you know that? I can resize the window by single pixel sizes and it seems to work just fine. I'll change it to always be multiples of 4 if that's actually the case, but then black borders from padding are unavoidable.

dyphire commented 2 years ago

Because those lines been removed, the scaler doesn't support scale to odd size, In some cases it can only be a multiple of 4. 5bb27e4#diff-6fa795ff14b0660affcdf013d87c0dd55c338000c8dffa297c5c0a350e1f713eL188-R17

It seems so. Even if the above changes are applied, when I test and adjust the thumbnail size, failure still occurs.

natural-harmonia-gropius commented 2 years ago

the scaler doesn't support scale to odd size

How do you know that? I can resize the window by single pixel sizes and it seems to work just fine. I'll change it to always be multiples of 4 if that's actually the case, but then black borders from padding are unavoidable.

In https://github.com/po5/thumbfast/pull/15 have a var named step, this function acturally port from my other vs filter, I had this problem, and I remembered.

natural-harmonia-gropius commented 2 years ago

0.666 diff and 2 pixels blackbar, weird. image image

Edit: result is 318x172 floor(172 / 1036 * 1920) maybe? image

dyphire commented 2 years ago

Interestingly, when the following patches are applied, the thumbnails will work.

--- a/thumbfast.lua
+++ b/thumbfast.lua
@@ -168,7 +168,7 @@ local function calc_dimensions()
     local scale = mp.get_property_number("display-hidpi-scale", 1)

     if width / height > options.max_width / options.max_height then
-        effective_w = math.floor(options.max_width * scale + 0.5)
+        effective_w = math.floor(options.max_width * scale - 0.5)
         effective_h = math.floor(height / width * effective_w + 0.5)
     else
         effective_h = math.floor(options.max_height * scale + 0.5)

I don't know why this patch seems to work well. Thumbnails can be generated normally in multiple thumbnail sizes on tested and compared.

christoph-heinrich commented 2 years ago

From what I remember the other thumbnailing script never had any black bars when I tested it a long time ago. But from a quick look at the code they aren't doing the padding thing with par to support those quirky discord memes :disappointed:

I have an idea that would respect that % 4 == 0 rule and still avoid black bars! I'll have time for that in ~30 mins.

christoph-heinrich commented 2 years ago

Please throw everything you have at it and tell me if there are any errors or black bars. I'm uncertain about the rounding of render_h. Simply going with math.floor would be the safer option, but I don't want to cut off any real pixels. Also it currently uses a step size of 4, but reducing it to 2 would be preferable if that doesn't cause any problems.

diff --git a/thumbfast.lua b/thumbfast.lua
index 4c96c9b..36999d0 100644
--- a/thumbfast.lua
+++ b/thumbfast.lua
@@ -68,8 +68,9 @@ local last_request_time = nil
 local last_display_time = 0

 local effective_w = options.max_width
-local effective_h = options.max_height
-local thumb_size = effective_w * effective_h * 4
+local generate_h = options.max_height
+local render_h = generate_h
+local thumb_size = effective_w * generate_h * 4

 local filters_reset = {["lavfi-crop"]=true, crop=true}
 local filters_runtime = {hflip=true, vflip=true}
@@ -154,7 +155,7 @@ local function vf_string(filters, full)
     end

     if full then
-        vf = vf.."scale=w="..effective_w..":h="..effective_h..par..",pad=w="..effective_w..":h="..effective_h..":x=(ow-iw)/2:y=(oh-ih)/2,format=bgra"
+        vf = vf.."scale=w="..effective_w..":h="..generate_h..par..",pad=w="..effective_w..":h="..generate_h..":x=(ow-iw)/2:y=(oh-ih)/2,format=bgra"
     end

     return vf
@@ -168,14 +169,17 @@ local function calc_dimensions()
     local scale = mp.get_property_number("display-hidpi-scale", 1)

     if width / height > options.max_width / options.max_height then
-        effective_w = math.floor(options.max_width * scale + 0.5)
-        effective_h = math.floor(height / width * effective_w + 0.5)
+        effective_w = math.floor(options.max_width * scale / 4 + 0.5) * 4
+        generate_h = math.ceil(height / width * effective_w / 4) * 4
+        render_h = math.floor(height / width * effective_w + 0.5)
     else
-        effective_h = math.floor(options.max_height * scale + 0.5)
-        effective_w = math.floor(width / height * effective_h + 0.5)
+        local target_h = math.floor(options.max_height * scale + 0.5)
+        effective_w = math.floor(width / height * target_h / 4 + 0.5) * 4
+        generate_h = math.ceil(height / width * effective_w / 4) * 4
+        render_h = math.floor(height / width * effective_w + 0.5)
     end

-    thumb_size = effective_w * effective_h * 4
+    thumb_size = effective_w * generate_h * 4

     local v_par = mp.get_property_number("video-out-params/par", 1)
     if v_par == 1 then
@@ -186,9 +190,10 @@ local function calc_dimensions()
 end

 local function info()
-    local display_w, display_h = effective_w, effective_h
+    local display_w, display_h = effective_w, render_h
     if mp.get_property_number("video-params/rotate", 0) % 180 == 90 then
-        display_w, display_h = effective_h, effective_w
+        print('rotato')
+        display_w, display_h = render_h, effective_w
     end

     local json, err = mp.utils.format_json({width=display_w, height=display_h, disabled=disabled, socket=options.socket, thumbnail=options.thumbnail, overlay_id=options.overlay_id})
@@ -395,7 +400,7 @@ local function thumb(time, r_x, r_y, script)
         last_index = index
         if x ~= last_x or y ~= last_y then
             last_x, last_y = x, y
-            display_img(effective_w, effective_h, time, mp.get_time(), script, true)
+            display_img(effective_w, render_h, time, mp.get_time(), script, true)
         end
         return
     end
@@ -409,13 +414,13 @@ local function thumb(time, r_x, r_y, script)
     if not spawned then
         spawn(seek_time)
         if can_generate then
-            display_img(effective_w, effective_h, time, cur_request_time, script)
-            mp.add_timeout(0.15, function() display_img(effective_w, effective_h, time, cur_request_time, script) end)
+            display_img(effective_w, render_h, time, cur_request_time, script)
+            mp.add_timeout(0.15, function() display_img(effective_w, render_h, time, cur_request_time, script) end)
             end
         return
     end

-    run("async seek "..seek_time.." absolute+keyframes", function() if can_generate then display_img(effective_w, effective_h, time, cur_request_time, script) end end)
+    run("async seek "..seek_time.." absolute+keyframes", function() if can_generate then display_img(effective_w, render_h, time, cur_request_time, script) end end)
 end

 local function clear()
@@ -430,7 +435,7 @@ end

 local function watch_changes()
     local old_w = effective_w
-    local old_h = effective_h
+    local old_h = generate_h

     calc_dimensions()

@@ -438,7 +443,7 @@ local function watch_changes()
     local rotate = mp.get_property_number("video-rotate", 0)

     if spawned then
-        if old_w ~= effective_w or old_h ~= effective_h or last_vf_reset ~= vf_reset or (last_rotate % 180) ~= (rotate % 180) or par ~= last_par then
+        if old_w ~= effective_w or old_h ~= generate_h or last_vf_reset ~= vf_reset or (last_rotate % 180) ~= (rotate % 180) or par ~= last_par then
             last_rotate = rotate
             -- mpv doesn't allow us to change output size
             run("quit")
@@ -457,7 +462,7 @@ local function watch_changes()
             end
         end
     else
-        if old_w ~= effective_w or old_h ~= effective_h or last_vf_reset ~= vf_reset or (last_rotate % 180) ~= (rotate % 180) or par ~= last_par then
+        if old_w ~= effective_w or old_h ~= generate_h or last_vf_reset ~= vf_reset or (last_rotate % 180) ~= (rotate % 180) or par ~= last_par then
             last_rotate = rotate
             info()
         end
dyphire commented 2 years ago

Please throw everything you have at it and tell me if there are any errors or black bars. I'm uncertain about the rounding of render_h. Simply going with math.floor would be the safer option, but I don't want to cut off any real pixels.

After applying this patch, everything seems normal to me.

By the way, when I test the size of some strange thumbnails, I will get black bars, but I think it is insignificant.

christoph-heinrich commented 2 years ago

I've noticed a problem that I'm pretty sure is responsible for causing those black bars at the top. It's because the thumbnail gets centered in the generated thumbnail. It used to not be the case, so now I have to find what change caused that and undo it, because that's essential for my approach here.

Edit: Turns out it already was centered a while ago, but I found what I had to change. I'll make a PR, because that's easier to test then a patch I think.

christoph-heinrich commented 2 years ago

Please test #29 and report any black bars or crashes. In case of problems you might want to insert print(effective_w, generate_h, render_h) into calc_dimensions() to get a look at the numbers.

dyphire commented 2 years ago

Please test #29 and report any black bars or crashes. In case of problems you might want to insert print(effective_w, generate_h, render_h) into calc_dimensions() to get a look at the numbers.

The new pr looks good, and no crashes and black bars appear in my tests.

po5 commented 2 years ago

this function acturally port from my other vs filter, I had this problem, and I remembered.

This never made sense to me in the context of vf scale, because we convert to bgra which shouldn't have mod2 restrictions (no chroma subsampling). We may need to convert to full-size chroma before calling scale.

christoph-heinrich commented 2 years ago

I've asked on the IRC about this, and the answer was

<avih> no, but it could also depend on the image format and/or how the chroma is arranged
<avih> i'm guessing there's no inherent internal limit, but that's a guess
christoph-heinrich commented 2 years ago

I've had some new insight on this, see https://github.com/po5/thumbfast/pull/29#issuecomment-1257083021. Still no idea how to properly solve it though.

christoph-heinrich commented 2 years ago

Can you please test #31 and see if it helps?

dyphire commented 2 years ago

Can you please test #31 and see if it helps?

these black bars appeared in my test.

christoph-heinrich commented 2 years ago

these black bars appeared in my test

Those seem unavoidable in some cases unfortunately. It's better to have a small border then no thumbnail at all, which would be the alternative. If there was a 1px black border at the bottom, we might be able to cleverly detect that and cut it out, but at the right side that's not possible. Maybe someone can think of a way of crafting requested resolutions that always result in either no border, or a border at the bottom, but as it is now, that's unavoidable. (actually, didn't the PR I closed do that? maybe I'll have a look at that later)

natural-harmonia-gropius commented 2 years ago

@dyphire Can you test https://github.com/po5/thumbfast/pull/28? Sometimes there is 1 pixel on the bottom edge under my test, but I think that's acceptable. But I do not know if it works properly under hidpi, Or there are also cases where there is no horizontal black border.

dyphire commented 2 years ago

actually, didn't the PR I closed do that? maybe I'll have a look at that later

I did not see the black bars when I use that closed pr, but it may be that I did not test enough samples.

dyphire commented 2 years ago

@dyphire Can you test #28? Sometimes there is 1 pixel on the bottom edge under my test, but I think that's acceptable. But I do not know if it works properly under hidpi, Or there are also cases where there is no horizontal black border.

It works under hidpi, and also had black bars on the bottom edge under my test.

po5 commented 2 years ago

Github automatically closed this, please confirm it's fixed.

dyphire commented 2 years ago

Github automatically closed this, please confirm it's fixed.

It works. Although thumbnails sometimes generate black bars, it is irrelevant to this issue.