Passive thumbnailer discussion

[N-R-K]

Moving the discussion from #17518 to not pollute the PR.

TL;DR a passive model where a client can request a thumbnail and the thumbnailer simply sends it back (asynchronously) rather than drawing the thumbnail itself would be better as it would:

• Avoid any desync issue between ui drawing vs thumbnailer drawing.
• Allow for other usecases such as multiple thumbnails (storyboard) or preview thumbnails for the entire playlist etc.

At a minimum such api should support the following "inputs":

1. Accept (optional) filename (currently playing file if none)
2. Accept (optional) "preferred" width and height
3. Accept timestamp hint for where to generate the thumbnail (note that the actual thumbnail doesn't have to be exactly on that timestamp, e.g a thumbnailer might be pulling from pregenerated sources and can only send back the closest match).
4. Accept an arbitrary "ID" from the user which will be sent back unmodified on the reply. This is mainly so that clients can easily figure out which reply corresponds to which request.

And reply back with the following:

1. Status: success or failure. None of the fields below (except ID) need be set on failure.
2. ID set by the user on request.
3. (bgra) thumbnail itself (see considerations below).
4. Actual w/h of the returned thumbnail. (Maybe stride as well since overlay-add expects it?)
5. Actual timestamp of the returned thumbnail.

Depending on the storage used to send the thumbnail (e.g filesystem) there might be some need for the client to inform the thumbnailer when it's done with the thumbnail and the thumbnailer is free to remove the thumbnail (if it wishes).

Additionally, you'd want to cancel in-flight requests as well, e.g user changed hover position so the old request is not needed anymore. (Note that the thumbnailer might still end up generating the thumbnail anyways, for caching purposes or otherwise, point is to inform the thumbnailer so that it can avoid generating the thumbnail if it doesn't need/want to).

Considerations

• Concerns were raised about using filesystem to share thumbnail as they are susceptible to TOCTOU and other usual filesystem shenanigans.
• overlay-add can accept both fd and memory address but these are not easy (?) to deal with in lua.

Testing

Because this would be a new api (as opposed to #17518 which is based on thumbfast api) this would require implementing it on 3rd party scripts first and refining the api based on real world usage before it can be considered for inclusion in mpv.

From the UI side, I am willing to have branches in mfpbar for testing purposes.

From non UI usecases, porting scripts like mpv-gallery-view to such api could provide good insights.

As for thumbnailer, we can either modify thumbfast, the yt-thumbnailer in #17518 or mpv_thumbnail_script.

Feel free to add anything else I missed here.

[CogentRedTester]

I have spent the last couple of days working on a (very early draft) Lua API that can be loaded by UI scripts to abstract away some of the more complex challenges of such an API, which I have been playing around with in this branch. It is just for testing purposes, so mpv isn't configured to compile with it, and there is no JavaScript version, but it is working and can be tested by manually loading thumbnail.lua. I have configured a version of osc.lua to use the API to perform the equivalent of your patches in #17518. I have also configured a version of thumbfast to make use of the API for testing purposes.

So far I have currently only tested using this API to replicate the behaviour of the "active" model in #17518. This is because thumbfast's code architecture is not designed to allow multiple requests at once, and I haven't had the time to try and add support for that. The API, as implemented, should allow such thumbnailers to be written, we just need to spend the time to do so, though it's possible that we'll realise a flaw in the design once we actually try.

For discussion's sake, and to report what I've learned, I'll briefly go through the API, compare it with the considerations and minimum requirements you've presented. Obviously this may not be the direction we go with the API, but even then, it should be useful for testing and experimentation.

Especially relevant pings: @N-R-K @po5.

Edit: this ended up not being brief at all, sorry if it's too long.

The Async API

At a minimum such api should support the following "inputs":

1. Accept (optional) filename (currently playing file if none)
2. Accept (optional) "preferred" width and height
3. Accept timestamp hint for where to generate the thumbnail (note that the actual thumbnail doesn't have to be exactly on that timestamp, e.g a thumbnailer might be pulling from pregenerated sources and can only send back the closest match).
4. Accept an arbitrary "ID" from the user which will be sent back unmodified on the reply. This is mainly so that clients can easily figure out which reply corresponds to which request.

I figure that for a first draft, we want the API to guarantee one response for each request. To guarantee that, we can do something like mp.input; we pass all the options to the thumbnailer, along with the name of the script and a unique script message string to handle the reply.

function thumbnail.generate(opts, cb)
    opts.id = opts.id or ''

    local thumbnailer = choose_thumbnailer(opts.path, opts.t)
    if not thumbnailer then
        return false
    end

    -- registers a unique script message to handle the reply
    local response_handler = register_response_handler(opts, cb)
    mp.commandv('script-message-to', thumbnailer, 'generate-thumbnail', utils.format_json({
        t = opts.t, w = opts.w, h = opts.h,
        path = opts.path, id = mp.get_script_name()..'/'..(opts.id or ''),
        client_name = mp.get_script_name(),
        response_handler = response_handler,
    }))

    return true
end

There is an extra id field that the thumbnailer can use to cancel or batch old requests, but currently there is an expectation that the thumbnailer will still send a "no success" response to those requests. This is behaviour we may want to reconsider, but there are potential advantages to guaranteeing that every call to generate a thumbnail responds with a request of some kind. For thumbfast I literally just cancelled the old request when a new one with the same id was received, and it didn't seem to cause any issues.

And reply back with the

1. Status: success or failure. None of the fields below (except ID) need be set on failure
2. ID set by the user on request
3. (bgra) thumbnail itself (see considerations below)
4. Actual w/h of the returned thumbnail. (Maybe stride as well since overlay-add expects it)
5. Actual timestamp of the returned thumbnail.

Currently I'm returning all of this except the id (since the unique script-message closures mean we always know anyway) and the actual timestamp. Adding new return fields would be trivial, though I confess I hadn't considered the actual timestamp too important, were there any particular usecases you had in mind?

(Maybe stride as well since overlay-add expects it)

Stride seems to always be w*4 right? So it doesn't really seem necessary. If any non-bgra format support (with a different stride calculation) were added to overlay-add, then we would need to add a new format field to the response, so adding a new stride field then, if necessary, would not add any extra burden on thumbnailers.

local function register_response_handler(opts, cb)
    local handler_id = mp.get_script_name().."/"..handle_counter
    handle_counter = handle_counter + 1

    mp.register_script_message(handler_id, function (response, err)
        mp.unregister_script_message(handler_id)
        response = utils.parse_json(response or '')
        if not response then
            return cb(nil, err)
        end

        local thumb = {
            w = response.w,
            h = response.h,
            _thumbnail = response.thumbnail,
            _status = 'available',
            _id = opts.id,
            _uid = handler_id
        }

        unfreed_thumbnails[handler_id] = thumb
        cb(setmetatable(thumb, thumbnail_mt), err)
    end)

    return handler_id
end

I don't know about these error messages, I suspect they won't be useful.

Cancelling requests

Additionally, you'd want to cancel in-flight requests as well, e.g user changed hover position so the old request is not needed anymore. (Note that the thumbnailer might still end up generating the thumbnail anyways, for caching purposes or otherwise, point is to inform the thumbnailer so that it can avoid generating the thumbnail if it doesn't need/want to).

I haven't implemented this (thumbfast returns thumbnails so fast it hasn't been noticeable), but it wouldn't be too difficult. In the context of this API, it probably makes more sense to cancel all requests of a particular id via some sort of thumbnail.abort(id) API. Alternatively, if we wanted to abort each individual request, we could do something more like:

local uid = thumbnail.generate({...})
thumbnail.abort(uid)
-- or:
uid:abort()

It's worth remembering though that aborted requests may still receive responses, if the responses were sent before the thumbnailer received the abort command. That is something that we ourselves could detect and block if we wished.

Resource Management

Thumbnail Data Transfer

Depending on the storage used to send the thumbnail (e.g filesystem) there might be some need for the client to inform the thumbnailer when it's done with the thumbnail and the thumbnailer is free to remove the thumbnail (if it wishes).

So far, I have been experimenting with doing memory management on the UI side; I have provided a method to free the data (calling os.remove() on the file).

    thumbnail.generate({id = 2, t = math.random(0, mp.get_property_number('duration') or 0)}, function(thumb)
        if not thumb then return end
        thumb:draw({x = 500, y = 300})
        thumb:free()
    end)

I had to modify thumbfast to write each thumbnail to a new temporary file (using os.tmpname(), I'm not sure if it works on windows), and then sending that. It works quite well, I took some measurements, and there did not seem to be any noticeable increase in compute time caused by using multiple files instead of one. The advantage of this is that the UI does not need to communicate with the thumbnailer to remove the file. The downside is that if a thumbnailer caches files (pre-emptively or no) it needs to make a copy of each before sending it to the UI. There is the potential to do both however; it would be trivial to allow the thumbnailer to tell the UI script (via an additional option in the response) that it wants to manage the file. Our :free() method could then automatically detect this flag and send a script-message back to the thumbnailer instead of deleting the file.

Additional Considerations

overlay-ids

One consideration that has not been mentioned is how to handle overlay-id conflicts. In the passive model, each UI script is drawing its own overlays, so there is a risk of multiple selecting the same ids and interfering with each other. Unless each client is given its own id namespace in the future (#17534), this is an issue for all thumbnail users.

Currently, to avoid overlay id conflicts, I see three options:

1. Let the UI clients pick their own (what happens now). This relies on the script authors or users to prevent any overlap, which is not ideal. Plus, something like a "gallery" script or a file/playlist browser may want to show tens of thumbnails at once, which may make overlapping id ranges impossible to avoid.
2. Have some kind of overlay-id allocator which UI scripts can query to loan an overlay-id assigment. This seems unideal to me, it's overly complicated, adds more latency to drawing thumbnails (due to more async requests), and it's unclear where such an allocator would actually exist (I don't think the thumbnailers should be responsible for this).
3. Have UI scripts use user-data to reserve overlay-ids.

This third option seems the simplest and is the one I've implemented it in the mp.thumbnail API:

-- Assigns overlay ids in such a way as to minimise the risk of conflicts with
-- other scripts. If the overlay ids were ever made client-specific, this function
-- could be simplified.
local function assign_overlay_id(thumb)
    if overlay_ids[thumb._id] then
        local reservation = mp.get_property_native('user-data/mpv/overlay-ids/'..overlay_ids[thumb._id])
        if reservation == mp.get_script_name() then
            return true
        else
            overlay_ids[thumb._id] = nil
        end
    end

    for i = OVERLAY_ID_MIN, OVERLAY_ID_MAX, 1 do
        local overlay_reservation = 'user-data/mpv/overlay-ids/'..i
        if not mp.get_property(overlay_reservation) then

            -- This is intended to (as far as possible) avoid two scripts from setting the same
            -- overlay id reservation at the same time. Still needs testing to determine if it works.
            mp.commandv('expand-properties', 'set', overlay_reservation,
                        ('${%s:%s}'):format(overlay_reservation, mp.get_script_name()))

            if mp.get_property_native(overlay_reservation) == mp.get_script_name() then
                overlay_ids[thumb._id] = i
                return true
            end
        end
    end

    -- If we reach here then we ran out of available overlay IDs
    return false
end

I am calling this for every thumb:draw() call, and clearing the reservation when the overlay is closed (but not when the thumbnail resources are freed). This is because a script may want to generate/cache tens/hundreds of thumbnails, but only draw some of them at once. For example, in my file-browser script, I would probably try and generate thumbnails for every file in a directory (10s-100s), but only draw the ~15 or so that fit on the screen at any one time.

However, I suspect (though have not been able to confirm this in practice) that this operation is racy; if two scripts try and set the same id at the exact same time, they might end up reserving the same id, which is why I tried to set the property in a strange way and why I've got guard clauses on both sides of the assignment. If we were to do this, it might require some testing.

Multiple Thumbnailers

Another consideration that was brought up in the previous thread was support for multiple thumbnailers. I still think it is a good idea, and I have been experimenting with an interface for doing so. There are a few ways of doing it that I've considered, but this post is getting very long and I'm getting tired, so I'll just talk about the method I actually implemented.

Essentially, I have configured it so that the UI script chooses a valid thumbnailer for the specific thumbnail being requested. This can all be done internally by the mp.thumbnail API. The thumbnailers just need to set a user-data field containing a specification of the types of thumbnails that it can generate:

mp.set_property_native('user-data/mpv/thumbnailers/'..mp.get_script_name(), {
    priority = 50,
    paths = {
        "^/",                           -- linux root
        "^[A--Z]:/",                    -- windows drives
        "^https?://youtube.com/",
        "^https?://[^/]+.youtube.com/", -- any youtube.com subdomain
        "^https?://yt.be/"
    },
    ranges = {
        {0, 60},
        {600, 99999}
    },
    current_file_only = true,
})

The priority value allows mp.thumbnail to choose which one to pick if there are multiple that can handle a thumbnail. Since there is no fallback behaviour, we need the thumbnailers to be able to describe their requirements sufficiently. I've done some brainstorming and I've come up with three useful criteria:

1. Whether the thumbnailer only works on the current file.
2. The paths/urls to generate thumbnails from.
3. Timestamp ranges. For example, the demuxer-cache thumbnailer suggested in the previous thread could update the ranges list with the seekable ranges specified in the demuxer-cache-state property.

This has actually worked very well in my testing. Another advantage of this, is that it can provide some immediate (synchronous) feedback to the UI script about whether to expect a thumbnail by returning false from thumbnail.generate() if there are no available thumbnailers. I used this in the modified OSC to only draw the thumbnail border if the function returned true.

String Pattern Problems

However, there is one big challenge to this, and that is Lua patterns and JavaScript regex do not use the same syntax and are not compatible with each other. So to handle this we would need to agree on some sort of custom/cut down version of either lua patterns or regex (or a hybrid or something else entirely) and convert it into a valid Lua pattern in Lua clients, and a valid regex pattern for javascript clients (or implement a custom parser but that's waaaaay out of scope). I spent some time on this, since I wanted to make sure it was possible to avoid going down a dead end, and it is actually possible to take a cut-down version of regex, and have it work on both lua and js after some processing, without it being too complex, code-wise (javascript version is identical in length):

for i, path in ipairs(config.paths or {}) do
    -- We need to use a custom pattern format that can work in both Lua and JS
    config.paths[i] = path:lower()
                          :gsub("\\[\\%-%*%?%[%]]", {       -- escape (our) special chars
                            ["\\\\"] = "\0a",   ["\\-"] = "\0b",
                            ["\\*"] = "\0c",    ["\\?"] = "\0d",
                            ["\\["] = "\0e",    ["\\]"] = "\0f",
                            ["\\^"] = "\0g",    ["\\$"] = "\0h",
                            ["\\+"] = "\0i"
                          })
                          :gsub("\\", "")                   -- remove backslashes from anywhere else
                          :gsub("([%(%)%%%.%-])", "%%%1")   -- escape lua special chars

                          :gsub("%%%-%%%-", "-")
                          :gsub("%*%?", "-")
                          :gsub("([+?])%?", "%1%%?")
                          :gsub("%[^%]", ".")

                          :gsub("%z%a", {                   -- Re-add our escaped characters
                            ["\0a"] = "\\",     ["\0b"] = "%-",
                            ["\0c"] = "%*",     ["\0d"] = "%?",
                            ["\0e"] = "%[",     ["\0f"] = "%]",
                            ["\0g"] = "%^",     ["\0h"] = "%$",
                            ["\0i"] = "%+"
                          })
end

So it's possible, and it doesn't take all that much code to implement, but is it actually something we would want to do for an API like this?

"Active" API Wrapper

Another thing I mentioned on the previous thread was that you could actually provide a wrapper API to achieve the "draw here" behaviour of the "active" model using the "passive" model on the backend. I wanted to test this and compare it with the active model, as I figured it would be a good way of testing the performance hit/latency of the passive model.

I initially wrote something that looked like this:

-- If a client clears the thumbnail, we don't want an in-transit thumbnail
-- response to redraw the image.
local blocked_draws = {}

function thumbnail.draw(opts)
    opts.id = opts.id or ''

    if not opts.x and not opts.y then
        clear_thumbnail(opts.id)
        blocked_draws[opts.id] = true
        return
    end
    blocked_draws[opts.id] = false

    return thumbnail.generate(opts, function(thumb)
        if not thumb then return end
        if not blocked_draws[opts.id] then
            thumb:draw(opts)
        end
        thumb:free()
    end)
end

And I tested it against the thumbfast branch of the OSC, which uses the base thumbfast API. Interestingly I was noticing major reductions in perceived latency of the image, the thumbfast version followed my mouse really smoothly, while this version was always a little behind. I did various tests and eventually realised that the difference was that thumbfast was updating the position of existing thumbnails while generating new ones, so I modified the draw API to cache the latest thumbnail and do this as well with an additional option:

-- If a client clears the thumbnail, we don't want an in-transit thumbnail
-- response to redraw the image.
local blocked_draws = {}
local latest_coords = {}
local latest_thumbnails = {}

function thumbnail.draw(opts)
    opts.id = opts.id or ''

    if not opts.x and not opts.y then
        clear_thumbnail(opts.id)
        blocked_draws[opts.id] = true
        latest_coords[opts.id] = nil
        if latest_thumbnails[opts.id] then
            latest_thumbnails[opts.id]:free()
            latest_thumbnails[opts.id] = nil
        end
        return
    end

    blocked_draws[opts.id] = false
    latest_coords[opts.id] = {x = opts.x, y = opts.y}
    if opts.redraw_immediately and latest_thumbnails[opts.id] then
        latest_thumbnails[opts.id]:draw({
            x = opts.x, y = opts.y,
            w = opts.w, h = opts.h,
        })
    end

    return thumbnail.generate(opts, function(thumb)
        if not thumb then return end

        if latest_thumbnails[opts.id] then
            latest_thumbnails[opts.id]:free()
        end
        latest_thumbnails[opts.id] = thumb

        if not blocked_draws[opts.id] then
            thumb:draw({
                x = opts.redraw_immediately and latest_coords[opts.id].x or opts.x,
                y = opts.redraw_immediately and latest_coords[opts.id].y or opts.y,
                w = opts.w, h = opts.h,
            })
        end
    end)
end

Once I made this change, there was no perceivable difference between thumbfast and this API. In addition, since the draw call still happens on the UI side, it is actually properly in sync with the rendered border:

                if thumbnail.draw({
                        t = mp.get_property_number("duration", 0) * (sliderpos / 100),
                        w = math.floor(thumb_w + 0.5), h = math.floor(thumb_h + 0.5),
                        x = math.floor(thumb_x + 0.5), y = math.floor(thumb_y + 0.5),
                        redraw_immediately = true,
                }) then
                    elem_ass:new_event()
                    elem_ass:pos(thumb_x * r_w, thumb_y * r_h)
                    elem_ass:an(7)
                    elem_ass:append(osc_styles.timePosBar)
                    elem_ass:append("{\\1a&H20&}")
                    elem_ass:draw_start()
                    elem_ass:rect_cw(-thumb_pad * r_w, -thumb_pad * r_h, (thumb_w+ thumb_pad) * r_w, (thumb_h + thumb_pad) * r_h)
                    elem_ass:draw_stop()
                else
                    thumbnail.draw({})
                end

Remaining Considerations

Obviously, the key advantages of the "passive" model (multiple requests simultaneously, different files) remains untested. So the next steps will be to modify some thumbnail scripts to use this API and see how it works in practice. I'll be out of town for a few days though, so I won't be able to make any progress for at least a week.

[N-R-K]

Another consideration that was brought up in the previous thread was support for multiple thumbnailers. I still think it is a good idea, and I have been experimenting with an interface for doing so.

I'll read & respond to the rest of the post later, but this part is concerning for me.

I don't think it's a good idea at all. A unified thumbnailer can do things like limit the number of parallel processes spawned (yt-thumbnailer does this) and control other aspects of thumbnail generation much more easily since it'll all be local to the thumbnailer rather than foreign script.

Also consider the fact that the amount of complexity is going to be a major factor on whether such api will get merged into mpv or not. A single thumbnailer is significantly less complex (api wise) than trying to create not just one api for client to thumbnailer communication but another on top for thumbnailer to different source.

[CogentRedTester]

Also consider the fact that the amount of complexity is going to be a major factor on whether such api will get merged into mpv or not. A single thumbnailer is significantly less complex (api wise) than trying to create not just one api for client to thumbnailer communication but another on top for thumbnailer to different source.

I'm not sure what you mean by "thumbnailer to different source", but I will note that if we're using a lua/js API wrapper, then there will always be two APIs; the wrapper API that is provided to clients, and the async API used by mp.thumbnail to communicate with thumbnailers. Whether that's something worth committing to is definitely worth discussing, and probably should be put in main post of the thread as a point of consideration.

[CogentRedTester]

this part is concerning for me.

No need to get concerned, this is just an experiment 😆. I don't intend to try and push anything unless there's a broad consensus.

[N-R-K]

but I will note that if we're using a lua/js API wrapper

There's already concerns raised on the simple api, so this will likely generate even more controversy. If the consensus changes for some reason, and we (i.e mpv codebase) want to get heavily involved in this then maybe this would/could work. But unless that happens, it should just be client <-> thumbnailer api while keeping things as simple as possible.