Use of option this._endOnLastSourceEnd

icyield commented 5 years ago

I presume that this._endOnLastSourceEnd in videocontext.js (line 880) should in fact be this.endOnLastSourceEnd.

this._currentTime += dt * this._playbackRate; if (this._currentTime > this.duration && this._endOnLastSourceEnd) { //Do an update od the sourcenodes in case anything in the "ended" callbacks modifes currentTime and sources haven't had a chance to stop.

PTaylour commented 5 years ago

this._endOnLastSourceEnd is set via the option: options.endOnLastSourceEnd

https://github.com/bbc/VideoContext/blob/579cbd3ab00db521648930c862547cb93fb75569/src/videocontext.js#L74

Are you finding that you can't set that option?

icyield commented 5 years ago

I can set the option and of course it works. But the default option is ignored.

PTaylour commented 5 years ago

Ah yes, I see what you mean.

The constructor has defaults for each options, which are only used if no options are passed

    constructor(
        ...
        options = {
            preserveDrawingBuffer: true,
            manualUpdate: false,
            endOnLastSourceEnd: true,
            useVideoElementCache: true,
            videoElementCacheSize: 6,
            webglContextAttributes: {
                preserveDrawingBuffer: true,
                alpha: false
            }
        }
    )

A member endOnLastSourceEnd is set to true, but not used internal to this class and not updated for users of this class [bug number 1]

{
        this._canvas = canvas;
        let manualUpdate = false;
        this.endOnLastSourceEnd = true;
       ...

If provided options.endOnLastSourceEnd is set internally
If no options are provided a default is used

if an option is provided, options.endOnLastSourceEnd is undefined (defaults to falsey) [bug number 2]

  ...
    if ("manualUpdate" in options) manualUpdate = options.manualUpdate;
    if ("endOnLastSourceEnd" in options) this._endOnLastSourceEnd = options.endOnLastSourceEnd;

And you're experiencing bug number 2?

germain-gg commented 5 years ago

I opened a PR to update how the options default are handled. #139

PTaylour commented 5 years ago

Awesome, thanks @gsouquet

germain-gg commented 5 years ago

@icyield Did that solve your issue? I guess we can close this issue now.

PTaylour commented 5 years ago

Shall close now. Feel free to reopen @icyield

bbc / VideoContext

Use of option this._endOnLastSourceEnd #135