wavesjs / waves-ui

A library to display and edit audio data and timeseries data in the browser.
http://wavesjs.github.io/waves-ui/
BSD 3-Clause "New" or "Revised" License
122 stars 16 forks source link

Moving a waveform segment (performance) #12

Closed echo66 closed 9 years ago

echo66 commented 9 years ago

Greetings!

Currently, the 'develop' branch allows the user to drag a waveform segment but, at each drag, the code seems to loop through all buffer positions. Wouldn't it be better to just loop through those points after stretching or moving the waveform beyond the visible area?

b-ma commented 9 years ago

Hey @echo66 ,

Actually, just the part of the buffer that is in the visible area is looped through, it's a change I have made last week, cf. : https://github.com/wavesjs/ui/blob/develop/es6/shapes/waveform.js#L79 I think this works quite well, if you think I missed something and that we parse more buffer than we should, could you be more specific ?

Actually, a performance problem with the waveform that we still have to deal with, is creating a subsampled version for long buffers.

Thx

echo66 commented 9 years ago

If you add, at the beginning of the update function (in waveform.js) something like this:

if(this.old) {
  const rc = renderingContext;
  if (rc.height == this.old.height && 
      rc.width == this.old.width && 
      rc.offsetX == this.old.offsetX && 
      rc.trackOffsetX == this.old.trackOffsetX && 
      rc.visibleWidth == this.old.visibleWidth ) {
    console.log("Just moving");
    return;
  }
}
this.old = {
  height: renderingContext.height, 
  width: renderingContext.width, 
  offsetX: renderingContext.offsetX, 
  trackOffsetX: renderingContext.trackOffsetX, 
  visibleWidth: renderingContext.visibleWidth
};

You won't need to run through the buffer data if you just move the waveform segment.

b-ma commented 9 years ago

Oh ok I see, actually as the waveform is an 'entity' layer, if you moves the waveform, you moves the layer itself and not the shape inside the layer. So if you call :

waveformLayer.updateContainer()

instead of :

waveformLayer.update()

which is just a shortcut for layer.updateContainer and layer.updateShapes, you will not render the waveform again.

You can also do this from the timeline :

timeline.tracks.updateContainers(waveformLayer)

But maybe we should had the kind of check you are proposing for the waveform, even if I feel this should be the programmer concern/responsibility to not update the shapes when not needed, it's also true that the waveform is the more expensive shape to compute and it should not render if not needed. This is something we should talk with @ouhouhsami.

So we keep this issue open for the moment.

Thx for your feedback

b-ma commented 9 years ago

Hey,

after discussion, we decided to not include this proposal in the Waveform, this is for two main reasons:

Actually, I fixed some problems in the rendering strategy I mentioned above and have not seen a performance problem yet (so patch a problem that does not yet exists, could looks like early optimisation). Again, the main problem IMO could still be with large audio files, which we should probably down sample in some way.

Thx