chrisguttandin / timingsrc

A library to synchronize a MediaElement with a TimingObject.
MIT License
31 stars 4 forks source link

Optimize createSetCurrentTime to avoid setting currentTime while seeking #22

Open AlexanderArvidsson opened 3 months ago

AlexanderArvidsson commented 3 months ago

Hi,

We optimized our non-sync player by avoiding to seek while the player is currently seeking, as outlined in this article: https://kitchen.vibbio.com/blog/optimizing-html5-video-scrubbing/

We found an extreme improvement in scrubbing performance when avoiding setting currentTime if the player is currently seeking, by listening to the "seeking" and "seeked" events. If currentTime is set while player is currently seeking, we instead store the "pending" seek time and set it after "seeked" as occured, effectively throttling based on the browser seeking time.

It might seem insignificant, but it does make a huge difference. My assumption (as well as the article) is that excessively setting currentTime causes the player to cancel fetching the frame from memory before it has fully loaded, instead moving on to fetch a different frame. This causes the player to "get stuck" while seeking, until you stop seeking. By waiting for "seeked" event, you can see frames pop up when you scrub, and you respect the loading times of your browser.

Since you support creating a custom timingsrc function, I can try implementing this in user space and it could later be provided as an alternative function in the library. This should not affect playback, since currentTime is avoided as much as possible to avoid stuttering, so it's up to you if you want to include this logic in the default timingsrc function, or an alternative.

Thoughts?

chrisguttandin commented 3 months ago

Hi @AlexanderArvidsson,

thanks a lot for suggesting this. It looks like this is a reasonable improvement. I tried to hack this into the video-synchronization-demo example. The seeking does indeed feel different afterwards but I can't necessarily tell that it's better or worse.

Do you have an example which clearly illustrates the difference? Maybe it also depends on the video codec or the network speed.

I'm also not sure how to implement it in a clean way. The hypothesis of the article is that the visible frame never gets updated when the loading of the previously requested frame gets canceled before it can be shown. But when the user ends scrubbing this is actually the desired behaviour. Otherwise it would unnecessarily delay the final update.

AlexanderArvidsson commented 3 months ago

Hi @chrisguttandin

Thanks for looking into this, really appreciated!

While you're right that it's desired that the loading of the previous frame should be cancelled, the hypothesis of the article (as I read it) is to avoid spamming the memory when you know you're scrubbing, and only move the timestamp when the user has stopped moving. By spamming and intentionally cancelling load operations, you actually end up with more stuttering when you're scrubbing. I see your point though, in worst case when you stop scrubbing, you're waiting for the current load to finish before you initiate a new load, pointing to the final frame. But in my experience, this was by no means a big deal, since the improvement in scrubbing was far greater than the minor latency you have when you stop moving. Continuous scrubbing performance is preferred over minor latency on the very end, at least in the Film & TV & Cinematics industri I work in, where quickly overviewing frame by frame without stutter is extremely important. The changes we made solved that issue, but it doesn't work in our sync mode because this does not have the same optimisation. I'd be happy to have it as opt-in, if possible (as I mentioned originally, as a custom factory, but there's too many other parts that you also have to copy for it to work).

The network speed isn't applicable because this is not network latency, it's latency when fetching from local cache. You get much better scrubbing speed even when the full video is cached by using the method outlined in the article, since you're not hitting memory as often (there's a LOT of overhead when you initiate a load range request on a video file in cache).

I don't currently have a side by side demo, since we noticed a drastic change when we implemented this at our company. What I can say for sure is if you use a high bitrate video (up in the 30+ Mbps), in turn increasing your local memory loading footprint, you will see drastic improvements.

I'd be happy to throw together a side by side demo, but I'm not sure when I have time to do it. I could perhaps record a video of our platform when we have the naive scrubbing on vs off, and you should see a clear difference.

AlexanderArvidsson commented 3 weeks ago

Hi @chrisguttandin,

I'm getting back to this right now and I can't really understand how I'd be able to make a custom setTimingsrc which has all the default factories, except with a custom updateMediaElement. The default factories aren't exported, so there's no way to re-create the same setTimingsrc as the original it seems?

Would it be possible to just export all factories so people can mix and choose what they want? Right now, only the gradual one is exported it seems. In my case, I literally want to re-create the exact function as the normal setTimingsrc except change the logic for createSetCurrentTime with my own logic. In order to do that, all functions that make up the original must be exported. The only alternative I have right now is forking it and publishing it as a separate package, which I don't want to do :/

Then I can test out the differences on our platform and see the results myself.


Edit: I ended up with this mess, which works but isn't very clean:

import { animationFrame, on } from 'subscribable-things'
import { createUpdateStepwise } from 'timingsrc'
import { createComputeVelocity } from 'timingsrc/build/es2019/factories/compute-velocity'
import { createDefaultSetTimingsrc } from 'timingsrc/build/es2019/factories/default-set-timingsrc'
import { createSetPlaybackRate } from 'timingsrc/build/es2019/factories/set-playback-rate'
import { createSetTimingsrc } from 'timingsrc/build/es2019/factories/set-timingsrc'
import { createSetTimingsrcWithCustomUpdateFunction } from 'timingsrc/build/es2019/factories/set-timingsrc-with-custom-update-function'
import { createUpdateGradually } from 'timingsrc/build/es2019/factories/update-gradually'
import { createUpdateMediaElement } from 'timingsrc/build/es2019/factories/update-media-element'
import { createWindow } from 'timingsrc/build/es2019/factories/window'
import { determineSupportedPlaybackRateValues } from 'timingsrc/build/es2019/functions/determine-supported-playback-rate-values'
import { pause } from 'timingsrc/build/es2019/functions/pause'
import { play } from 'timingsrc/build/es2019/functions/play'

With this, I set up a side-by-side with the left side only setting currentTime when the player is not actively seeking, and the right is the default behavior. You can confirm this in Console, where the right always prints "update" and the left prints "update" and "seeked" (where updates are supressed when seeking and "seeked" indicating it's ready for next).

Firefox: https://github.com/user-attachments/assets/f74afdc0-b894-4cab-8619-86802ac093c4

Chrome: https://github.com/user-attachments/assets/b8d86b7e-e36d-470b-8827-2c32a49893f7

What happens is that the right is congesting due to memory lookups; the video is fully in memory and no network calls are being made. Because of the high frequency of memory calls, the video gets stuck until you "slow down", while in the left panel it only updates the current time when it knows that there's nothing going on. I demonstrate both 1080p and 480p on both Firefox and Chrome, where it's noticable on both browsers.

chrisguttandin commented 1 week ago

Sorry for the inconvenience. I'm glad you found a way to circumvent the missing exports.

Thank you for your perseverance. Your examples clearly show that there is a difference. I'm curious though why there wasn't any noticable difference in my tests. Would it be possible for you to share your test files? You can also do this via mail if you prefer not sharing it publically. I would love to set up an automated test to verify that making the code more complicated is actually worth it and that we know when to revert the changes again if browsers handle this internally at some point.

Again thanks a lot for taking the time to demonstrate the problem.

AlexanderArvidsson commented 1 week ago

Hi! No worries!

I actually found out why memory / decoding (and thereby scrubbing) is slow for my videos. Our transcoding parameters were not tuned well for scrubbing and has a lot of B/P frames with few I-frames. We tuned the compression and the GOP size and scrubbing is much much smoother now.

But even with these transcoder changes, the scrubbing is still smoother when using the method I outlined before. It just makes more sense to respect the time that the decoder needs to decode the scrubbed frame before trying to initiate a new one. If you don't, you're canceling a decode mid-way, so the frame won't display until you stop spamming it with new frames to decode. Decoding a highly compressed video stream, without proper GOP size, and with excessive B/P frames, will take time. We've been using my changed settimingsrc method now for a while internally and it's giving us no problems and much much smoother scrubbing than without it.

Here's the video files, before tuning and after tuning, if you want to give it another go: Before: https://github.com/user-attachments/assets/bbfe9341-f8bf-4941-a283-9dfabbed3164

After: https://github.com/user-attachments/assets/6cc19868-5435-4f87-a81e-a4a9497ad25f

Here's the params used:

"H264Settings": {
    **(
        {
            "FramerateControl": "SPECIFIED",
            "FramerateNumerator": transform.frame_rate,
            "FramerateDenominator": 1,
        }
        if transform.frame_rate
        else {
            "FramerateControl": "INITIALIZE_FROM_SOURCE"
        }
    ),
    "MaxBitrate": transform.bitrate_kbps
    * 10
    * 10
    * 10,
    "RateControlMode": "QVBR",
    "SceneChangeDetect": "TRANSITION_DETECTION",
    # Optimize for scrubbing
    "GopBReference": "DISABLED",  # No GOP B reference frames
    "GopClosedCadence": 1,  # All closed, better scrubbing
    "GopSize": 1,  # Use frame-rate as GOP size
    "GopSizeUnits": "SECONDS",
},

Even Firefox is struggling to quickly scrub in the before video. Try spam-clicking the timeline (drag doesn't work in Firefox to scrub continuously), and see that it's much faster in the After video. Now imagine your spam clicking is done so fast that you're interrupting the decoder, which causes the frame to cancel rendering until the next decode is done, which if you keep spamming will keep pushing the new frame over and over again.

Here's the createSetCurrentTime I wrote (let me know if there's any glaringly obvious mistake here):

export const createSetCurrentTime = (
  currentTimeAssignments: WeakMap<
    HTMLMediaElement,
    [prevTime: number, nextTime: number, seeking: boolean]
  >,
) => {
  const subscribedMap: WeakMap<HTMLMediaElement, VoidFunction> = new WeakMap()

  return (mediaElement: HTMLMediaElement, previousValue: number, nextValue: number) => {
    const currentTimeAssignment = currentTimeAssignments.get(mediaElement)

    if (!subscribedMap.has(mediaElement)) {
      const onSeeked = () => {
        const currentTimeAssignment = currentTimeAssignments.get(mediaElement)
        if (!currentTimeAssignment || !currentTimeAssignment[2]) return

        currentTimeAssignment[2] = false

        if (mediaElement.currentTime !== currentTimeAssignment[1]) {
          mediaElement.currentTime = currentTimeAssignment[1]
        }
      }

      mediaElement.addEventListener('seeked', onSeeked)
      subscribedMap.set(mediaElement, onSeeked)
    }

    if (currentTimeAssignment?.[2]) {
      currentTimeAssignment[0] = mediaElement.currentTime
      currentTimeAssignment[1] = nextValue

      return
    }

    if (
      currentTimeAssignment === undefined ||
      // Bug #5: Safari limits the precision of the value after a while.
      Math.abs(currentTimeAssignment[0] - previousValue) > 0.0001 ||
      currentTimeAssignment[1] !== nextValue
    ) {
      mediaElement.currentTime = nextValue

      currentTimeAssignments.set(mediaElement, [mediaElement.currentTime, nextValue, true])
    }
  }
}

I hope this helps!