Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the ProgressIndicator to be enabled/disabled #199

Merged
merged 3 commits into from
Mar 24, 2023

Conversation

renefs
Copy link
Contributor

@renefs renefs commented Mar 21, 2023

The behaviour is the same as the one in the PlaybackBar, so basically the user is able to hide or show the progress bar just by changing the value of the enabled attribute once the player is PLAYER_LOADED state:

player.bindEvent(player.Events.PLAYER_LOADED, () => {
    player.playbackBar.progressIndicator.enabled = false;
}, false);

If find this useful to live events when the user actions on the progress bar can affect negatively the stream playback.

I'm not sure if the white space between the PlaybackBar and the video container could be removed somehow, since it is a bit aesthetic.

Just let me know what you think.

Related issue #177

@renefs
Copy link
Contributor Author

renefs commented Mar 21, 2023

This is the white space I was referring to:

Screenshot 2023-03-21 at 15 51 38

@ferserc1
Copy link
Collaborator

This change interferes with progress indicator plugins and the current time indicator. The element that should be disabled is progress-indicator-container. Hiding this element will also prevent mouse events from seek seek the video.

Note that progress indicator plugins could be useful for live videos as well. For example, in addition to disabling the playback bar (by hiding progress-indicator-container) it would be possible to display a text instead indicating that it is a live event:
image

This would also eliminate the white space problem you mention.

However, the API you propose seems very interesting, as it could be used to implement a progress indicator plugin, which would also disable the time bar. Perhaps the name of the function should be changed to something more indicative, such as hideTimeLine()/showTimeLine().

export default class LiveStreamingProgressIndicatorPlugin extends ProgressIndicatorPlugin {
    async isEnabled() {
        const enabled = await super.isEnabled();
        return enabled && 
               this.player.videoContainer.isLiveStreaming;  // TODO: implement isLiveStreaming attribute
    }

    async init() {
        this.player.playbackBar.progressIndicator.hideTimeLine();
    }

    drawForeground(context, width, height, isHover) {
        context.fillStyle = "rgba(230, 230, 230)";
        context.font = `12px Arial`;
        context.textAlign = "left";
        context.fillText(this.player.translate("Live streaming"), 100, height / 2 + 5);
    }
}

If you can modify your implementation to hide only the playback bar and change the enabled/disabled functions by hideTimeLine/showTimeLine, I can add the isLiveStreaming method to simplify the identification of live events and add the LiveStreamingProgressIndicatorPlugin plugin in the paella-basic-plugins repository.

To hide the progress indicator text, you should hide the .progress-indicator-timer element, but I think that this element should be hidden using another API (maybe hideTimeText/showTimeText). It's possible that you want to prevent the user to seek in live streaming events, but maybe you want to show the current time.

@github-actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@renefs
Copy link
Contributor Author

renefs commented Mar 23, 2023

Looks fine to me. I have a working solution using a plugin, the ProgressIndicator and the VideoContainer.

Screenshot 2023-03-23 at 11 53 24

Screenshot 2023-03-23 at 11 53 16

Should we define on the video manifest metadata whether is live or not?

this._isLiveStreaming = this.player.videoManifest.metadata?.isLiveStreaming;

The plugin looks like:

export default class LiveProgressIndicatorPlugin extends ProgressIndicatorPlugin {

    get minHeight() {
        return 20;
    }

    get minHeightHover() {
        return 20;
    }
    async isEnabled() {
        const enabled = await super.isEnabled();
        return  enabled &&
               this.player.videoContainer.isLiveStreaming;
    }

    async load() {
        if(this.isEnabled()){
            this._hideProgressTimer = true; // This will be managed though the plugin's config in real version
            this.player.playbackBar.progressIndicator.hideTimeLine(this._hideProgressTimer);
        }
    }

    drawForeground(context, width, height, isHover) {
        let leftPadding = 80;
        if(this._hideProgressTimer){
            leftPadding = 10;
        }
        context.fillStyle = "#ff0000";
        context.beginPath();
        context.arc(leftPadding, height / 2, 5, 0, 2 * Math.PI);
        context.fill();
        context.font = `14px Arial`;
        context.textAlign = "left";
        context.fillText(this.player.translate("Live streaming"), 20+leftPadding, height / 2 + 5);
    }
}

If it looks ok to you, I can add the plugin to the paella-basic-plugins.

@ferserc1
Copy link
Collaborator

I'm working on adding a property to videoContainer that returns whether a video is a live stream. What this property does is determine if any of the video streams being played is of type hlsLive. You should cancel all changes you have made in VideoContainer, because I am preparing that part based on the current specification of the video manifests.

Maybe you should wait until I have this new property ready.

@github-actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@renefs
Copy link
Contributor Author

renefs commented Mar 23, 2023

Sure, no problem. I reverted the changes and left only the ones in ProgressIndicator. Do you prefer to cancel the PR and make on only for the plugin in paella-basic-plugins with the code in my comment above?

@ferserc1 ferserc1 merged commit 6db3bbb into polimediaupv:main Mar 24, 2023
@ferserc1
Copy link
Collaborator

I would appreciate if you could add the plugin pull request you mention above in paella-basic-plugins, but you will have to wait until I release paella-core version 1.25 and update paella-basic-plugins to use that version. I'm going to have it ready this morning, so you can start this afternoon.

For the pull request, note that plugin classes are exported in the index.js file, so that they can be extended by the user. For example:

index.js:

...
import CustomTimeProgressIndicator from './plugins/es.upv.paella.customTimeProgressIndicator';
...
export const CustomTimeProgressIndicatorPlugin = CustomTimeProgressIndicator;

Please add a description of the plugin to the paella-basic-plugins readme.md file, in the same way as the other plugins are described. Something like this:

Custom time progress indicator

Allows to add a video elapsed time indicator as a non-interactive button type plugin.

{
    "es.upv.paella.customTimeProgressIndicator": {
        "enabled": true,
        "textSize": "large",    // "small", "medium" or "large"
        "showTotal": false
    }
}

Exported as CustomTimeProgressIndicatorPlugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants