mediacms-io / mediacms

MediaCMS is a modern, fully featured open source video and media CMS, written in Python/Django and React, featuring a REST API.
https://mediacms.io
GNU Affero General Public License v3.0
2.75k stars 505 forks source link

video.js should only show specific resolutions as options #824

Open mgogoulos opened 1 year ago

mgogoulos commented 1 year ago

This is happening on vertical videos, video.js is showing the resolutions it has received as part of the video encodings (240/360/720/1080) BUT also it is showing the resolutions it got from the hls_file. This ends up in duplicate entries. To prevent, ensure that resolutions are amongst the valid list

Screenshot from 2023-06-27 13-57-00

Screenshot from 2023-06-27 13-56-43

mgogoulos commented 1 year ago

Hi, sure, but I don't see the comment on the issue anymore, and cannot assign you!

On Tue, Jun 27, 2023 at 6:50 PM AmitT98 @.***> wrote:

Hi @mgogoulos https://github.com/mgogoulos I can fix the issue, can you please assign it to me?

— Reply to this email directly, view it on GitHub https://github.com/mediacms-io/mediacms/issues/824#issuecomment-1609792163, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2YLAGM5HYX3D5OR4HQG3XNL6KXANCNFSM6AAAAAAZVNQSS4 . You are receiving this because you were mentioned.Message ID: @.***>

mgogoulos commented 1 year ago

So I got this request by AmiT98 through email but cannot see the comment here. It's either an automated account, or something similar I can only assume.

AmitT98 commented 1 year ago

Hello @mgogoulos sorry to bother. But I thought you are working on the issue as you self-assigned it. So I removed my comment. I would still like to work on the issue. If you can assign it to me it would be great.

AmitT98 commented 1 year ago

Hello Mediacms-Io/Mediacms, sorry but I thought that since you self-assigned it. You would have worked out the solution. But I would still be interested in working on the issue, if you assign it to me.

On Wed, Jun 28, 2023 at 1:09 AM Markos Gogoulos @.***> wrote:

Hi, sure, but I don't see the comment on the issue anymore, and cannot assign you!

On Tue, Jun 27, 2023 at 6:50 PM AmitT98 @.***> wrote:

Hi @mgogoulos https://github.com/mgogoulos I can fix the issue, can you please assign it to me?

— Reply to this email directly, view it on GitHub < https://github.com/mediacms-io/mediacms/issues/824#issuecomment-1609792163>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAH2YLAGM5HYX3D5OR4HQG3XNL6KXANCNFSM6AAAAAAZVNQSS4>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/mediacms-io/mediacms/issues/824#issuecomment-1610109539, or unsubscribe https://github.com/notifications/unsubscribe-auth/AY3YUB7BB22IKFRIMLLMQ6TXNMZGFANCNFSM6AAAAAAZVNQSS4 . You are receiving this because you commented.Message ID: @.***>

mgogoulos commented 1 year ago

Hi @AmitT98 , you can work on this issue (or any other...) and open a PR and we can discuss there, there's no need to be assigned to an issue in order to work on it. But I've assigned it to you if it helps.

Thanks

AmitT98 commented 1 year ago

Hi @mgogoulos , just a quick question, I was running through the website demo.mediacms.io and I didn't notice this issue. Does it occurs only in some specific cases?

mgogoulos commented 1 year ago

@AmitT98 this is happening with videos that are vertical - https://demo.mediacms.io/view?m=gNztKHL3R

Check the API response with the video metadata, on https://demo.mediacms.io/api/v1/media/gNztKHL3R

Video.js is showing the keys of encodings_info (1080, 720 etc) that represent video mp4 files. But if will also display the contents of hls_info (if it has contents), again the keys (striping the _iframe string, eg on 720_iframe it will get the 720). In the case of horizontal videos, keys are the same (anything or all between 240, 360, 720, 1080, 2160). But in the case of HLS file for vertical videos, it will get the extra keys and show it, with unexpected behaviour. I think a simple solution could be to check if a key is part of the standard keys (240, 360, 720, 1080, 2160) and if not don't show it...

AmitT98 commented 1 year ago

So I understand it correctly, this code (present in mediacms/frontend/packages/player/src/MediaPlayer.js) image populates pluginVideoResolutions object based on the resolutionKeys exported by mediacms/frontend/packages/vjs-plugin/dist/mediacms-vjs-plugin.js. image Correct?

mgogoulos commented 1 year ago

To be honest, I don't know :)

But I've also thought of a different approach here, which is to change the HLS info on the response API call.

https://github.com/mediacms-io/mediacms/pull/832 I'm giving this a try...

AmitT98 commented 1 year ago

Nice hack there @mgogoulos , I think this should be solved now.

mgogoulos commented 1 year ago

Nice hack there @mgogoulos , I think this should be solved now.

Thanks, it might not be the best way to handle it, but probably it's better than having all these extra numbers...