Sagnac / streamsave

mpv script aimed at saving live streams and clipping online videos without encoding.
92 stars 4 forks source link

Option to add chapters at A-B loop points #1

Closed Sagnac closed 3 years ago

Sagnac commented 3 years ago
This is currently experimental and requires mpv to be built with
mpv-player/mpv@6abb7e3 in order for the chapter marks to immediately
show on the OSC.

The new range_marks option allows the script to set temporary chapters
at A-B loop points. If chapters already exist they are stored
and cleared whenever any A-B points are set. Once the A-B points are
cleared the original chapters are restored. The script also attempts to
take into account any chapters added after A-B mode is entered by adding
them to the initial chapter list.

So I'm going to leave this here for a while in case anyone actually uses this script along with others which perform any significant chapter list manipulation and has any idea on how to handle this better.

Consideration of initial static chapters seems to be solved, although there's probably a more efficient way to do it.

The only way I can see this clashing is if another script attempts to delete set chapters or simply overwrites the chapter list. The former might be problematic as this option replaces the chapter list with the A-B loop points so the other script might lose access to the previous chapter list (assuming it's observing the chapter list and using the new table instead) until it's restored. The latter is problematic as this script currently assumes new chapters are added on top of (or rather, appended to) the current list and so the first 1-2 chapters could be lost if the list is overwritten instead.

In any case, if significant destructive operations are being performed on a dynamic chapter list I see no reason to be using the range_marks option anyway.

Also, I barely tested this so there might be a few bugs at the moment.

Sagnac commented 3 years ago

A few more thoughts regarding other scripts possibly interfering with this option,

The goal is to address the more general case instead of a specific script's implementation. I can think of two ways a script would change the chapter list:

  1. Get the current chapter list first, then modify it.

  2. Keep an internal table with the desired chapters, modify that as needed, then set the chapter list to it.

Method 2 is probably the most common as there's little need to keep the chapter list updated using multiple mp.get_property calls unless you expect something else to be changing the list. If this method is used then the current implementation in this script would result in a differing chapter list when the master list is restored. Making use of a shared script property which could be observed might be required here in order to make the scripts compatible.

Method 1 could lead to issues if the chapter order changes. This method would be used over method 2 if, for some reason, the added chapters are required to be updated immediately rather than waiting until the A-B points are unset. The workaround would require the other script getting the current chapter list and then simply appending the new chapters to it, forgoing any deletion or chapter list reordering as long as the A-B points exist. The workaround in this script currently addresses this strategy, as any chapters added after any of the A-B points are set are appended to the master chapter list.

I see little reason to add more code here addressing this as any clashing script would probably need to be modified anyway. In fact, the current implementation probably isn't necessary as this can be dealt with in the other script, but it at least makes things easier and eliminates the necessity to have two scripts observing the A-B loop properties if appending chapters is all that is desired.

Of course, I can just add the A-B loop chapters to any existing chapter list and that would fix some of the potential issues, but that sort of defeats the purpose of this option to begin with. Namely this is meant to

  1. Add visible indication on the OSC of the cached range the script will dump.
  2. Allow an easy way to move to the start and end points of this range without having to bind a key to seek to the A-B points as the chapter keys can be used instead.
  3. Provide a simple way to query the set range by right clicking the chapter buttons on the OSC.

So it makes sense to store the existing chapter list, clear it when "clipping mode" is entered, and then restore it when it's exited. The problem is if another script adds chapters after the A-B point chapters are set.

Another idea is to observe the chapter-list property and do something like

function on_chapter_change(_, list)
    if #ab_chapters ~= #list then
        chapter_list = list
    end
end
mp.observe_property("chapter-list", "native", on_chapter_change)

but this isn't perfect either in cases where the chapter list is overwritten with just 1-2 chapters, or when the appending method is used (in which case the A-B chapters would get added to the master list). Direct comparison of the tables also doesn't work as the hash values will differ.

Another approach would be to loop through the list and construct a table only if they're not the A-B chapters (could use the title field for this) but I'm not sure if this is worth the trouble of doing so.

Sagnac commented 3 years ago

Okay so now both cases should be covered.

This should make the option compatible with most scripts which change the chapter list and it only utilizes a few lines to do it. I still need to test it and make sure there are no bugs.

I'll merge this in a week or two if I can't find a better solution.