Improve flexibility of quality selector #1081

gurupras · 2018-07-01T00:49:08Z

I would like some feedback on this PR. If you use Plyr and you care about the quality setting, please have a read and provide feedback.

Link to related issue (if applicable)

This is related to #878 and also PR #988

Summary of proposed changes

All providers specify the following for each quality level
- label: String mandatory (This is what shows up in the menu. Solves Allow the quality to be a string #878)
- height: Number mandatory (Used to determine badge if badge isn't specified)
- index: Number optional
- badge: String optional

The 'index' field is optional and is used to resolve conflicts. An example of where this may come in handy is provided below.

Finally, the badge field allows the provider to explicitly specify a badge for a particular quality level. This is somewhat a double-edged sword. On one hand, this gives full freedom to providers to determine the badges for each quality level. On the other, it may lead to inconsistencies where one provider reports HD for a video with height of 720 whereas another provider does not.

Imagine a scenario where the available quality levels are as follows:

{width: 422, height: 180, bitrate: 258157}
{width: 638, height: 272, bitrate: 520929}
{width: 638, height: 272, bitrate: 831270}
{width: 958, height: 408, bitrate: 1144430}
{width: 1277, height: 554, bitrate: 1558322}
{width: 1921, height: 818, bitrate: 4149264}
{width: 1921, height: 818, bitrate: 6214307}
{width: 4096, height: 1744, bitrate: 10285391}

In the above example, with the current implementation, the only attribute that is passed on to the provider on qualityrequested is value. If the provider implemented this similar to how YouTube is currently implemented (purely based on height), then, there would be two quality entries with the same value. My hope is that the index field will help with resolution in such cases by telling the provider exactly which quality element was selected.

Even after #988 has been merged in, the quality badge relies heavily on some hard-coded values. If a video presents a quality value that is not found in [480, 576, 720, 1080, 1440, 2160], then no badge is provided. In the above example, none of the entries would get a badge (if the value was determined by height).
Instead, this PR allows providers to explicitly specify a badge of their choice. We could also add some fallback logic where the 'height' attribute is used to determine the badge.

Here's a pen that shows the results of this PR for HTML5/HLS/YouTube.

Checklist

Use develop as the base branch
Exclude the gulp build from the PR
Ensure PR makes sense with regard to i18n
Figure out how to solve initial quality on HTML5 videos (currently shows undefined)
Ensure plyr.quality = '...' works
Test on supported browsers

gurupras · 2018-07-01T00:49:56Z

@friday @philipgiuliani: Kindly take a look at this and provide feedback

sampotts · 2018-07-01T00:52:02Z

PR should really be against develop

gurupras · 2018-07-01T00:53:12Z

Ah, my bad. Updated.

sampotts · 2018-07-01T00:54:09Z

src/js/plyr.js

-            const value = closest(options, quality);
-            this.debug.warn(`Unsupported quality option: ${quality}, using ${value} instead`);
-            quality = value;
+        if (input instanceof String) {


Could you use the type checkers we have? e.g. is.string(input)

sampotts · 2018-07-01T00:54:17Z

src/js/plyr.js

+            quality = {
+                value: input,
+            };
+        } else if (input instanceof Object) {


Could you use the type checkers we have? e.g. is.object(input)

sampotts · 2018-07-01T00:54:37Z

src/js/plugins/youtube.js

                        set(input) {
-                            instance.setPlaybackQuality(mapQualityUnit(input));
+                            let label;
+                            if (input instanceof String) {


Could you use the type checkers we have? e.g. is.string(input)

sampotts · 2018-07-01T00:54:45Z

src/js/plugins/youtube.js

+                            let label;
+                            if (input instanceof String) {
+                                label = input;
+                            } else if (input instanceof Object) {


Could you use the type checkers we have? e.g. is.object(input)

sampotts · 2018-07-01T00:55:13Z

src/js/plugins/youtube.js


-    return dedupe(levels.map(level => mapQualityUnit(level)));
+    const mappedLevels = dedupe(levels.map(level => mapQualityUnit(level)));
+    const result = mappedLevels.map((level, index) => ({


This can just be an early return since you're returning it on the next line anyway.

sampotts · 2018-07-01T00:59:28Z

Thanks for your work on this 👍

Is this a breaking change?
Will the current options and markup work with this?

I think having the fallback would be needed to allow the minimum effort. Not everyone will want to pass their own badge text. We can use the closest() array util or just something like if (x > y) { badge = 'HD' } to determine the best badge to display but allow devs to override these. That would be the ideal implementation I'd say?

gurupras · 2018-07-01T01:04:37Z

I don't know how to be a 100% whether this is/isn't a breaking change.

Currently, on plyr.io, plyr.quality returns a Number, this implementation returns a String. In other words, this implementation returns the current quality label. This may count as a breaking change..

Also, at the moment, I just noticed that there seems to be a bug with the setter plyr.quality = '1080', I'll fix this ASAP.

friday · 2018-07-01T01:10:38Z

Thanks for this! I'll take a look at it soon. Can also check if it's breaking then. Can you fix the linting errors apart from the no-cycle ones (they were already there before)? They should be easy.

gurupras · 2018-07-01T01:18:37Z

src/js/plugins/youtube.js

+                            if (is.string(input)) {
+                                label = input;
+                            } else if (is.object(input)) {
+                                label = input.label;


Don't know how to fix this linting error.

Destructuring is when you do this:

const {label} = input;

Instead of this:

const label = input.label;

Since it's already declared here you need to wrap with parenthesis:

({label} = input);

friday · 2018-07-01T02:42:27Z

src/js/controls.js

+        // Set options if passed
        if (is.array(options)) {
-            this.options.quality = dedupe(options).filter(quality => this.config.quality.options.includes(quality));
+            this.options.quality = dedupe(options);


dedupe doesn't work on objects. It doesn't do "deep equals".

Shall I replace this with just

if (is.array(options) { this.options.quality = options }

(Sam might disagree but at least it won't look like it's doing something and then it doesn't work)

friday · 2018-07-01T02:59:12Z

src/js/plugins/youtube.js

+        label: levels[index],
+        index,
+        height: level,
+        badge: level >= 720 ? 'HD' : 'SD',


If you set the badge here the i18n won't work, right? With the fallback you don't need to anyway right?

Removing badge in favor of default logic

friday · 2018-07-01T03:28:59Z

src/js/html5.js

+            height: size,
+            index,
+        }));
    },


There are multiple problems here.

Not really a problem per se, but as Sam pointed out there's no need for the intermediate variable sizes. You can just add to the existing function chain.

The label will be 720 not 720p as I'm sure you intended it to be (since otherwise the template literals doesn't really make sense).

However I don't think you should set the label at all since later on in the code you need to be able to distinguish between data that was specified by developers and fallback data in order to respect the data specified by developers. I would use a class for the entries in this.options.qualities in order to solve this via getters and setters (moving the logic from getLabel and getBadge)

It wouldn't be possible to set the badge and label via the source element.

filter(Boolean) removes all falsy values. I think with the other changes in this PR this isn't something we want. Especially not before grabbing the array index, like now. Rather, we should do that when creating the menu.

I suggest you change it to something like this:

// I haven't tested this code, and the comments are meant for you, not to stay in the code return html5.getSources.call(this) .map((source, index) => { // Get data from the dataset (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset) const { // For backward-compatibility, fallback on the old size-attribute (which I think we should deprecate) height = source.getAttribute('size'), label, badge, } = source.dataset; return { index, // Really don't think we need index here at all, as mentioned badge, label, height, }; });

The template literal is to ensure that label conforms to the laid down rule (by me) that label is a String.

Updated with your suggestion. This is neat. I like being able to specify label, badge via data- attributes.

friday · 2018-07-01T03:35:36Z

src/js/plyr.js

     * @param {number} input - Quality level
     */
    set quality(input) {
        const config = this.config.quality;


The old quality setter code was already too complex and hard to understand, so I think this is a step in the wrong direction. I think you should restore the old setter and the getter in the html5/youtube plugins and add a new getter/setter that sets only by index. videoTrack or level perhaps?

Part of your comment, I think, is referring to the fact that this setter is trying to account for number/string/object all-in-one.

The number version was added just now to be backwards compatible. At least with plyr.io. It wasn't meant to handle index.

The string version is to cater to users wanting to set it by label.

The object version is mainly to handle the internal implementation wherein:
user clicks quality option --> translate event.target.value into quality somehow. This was where the object came into play.

Now, with the suggestion to remove index attribute and use it as the value while creating radio button, we can update the quality setter to receive, for example, either ["0", 0, { index: 0 }]. I opted for the object implementation to allow sufficient differentiation.

friday · 2018-07-01T03:55:23Z

src/js/controls.js

+                    // TODO: Try to find a badge based on height
+                    return null;
+                }
            }


This can be done a lot simpler with destructuring and default parameters.

Shouldn't it be quality.height. Not quality.label?

Suggestion (not tested):

const getBadge = quality => { const { height, badge = i18n.get(`qualityBadge.${height}`, this.config); } = quality; return badge ? controls.createBadge.call(this, label) : null; };

Although badge should be based on height in theory, piggy-backing on the existing i18n requires the height to be 480. A value of 482 (if it were the case) would generate no badge despite being the real height of the video. I think I will update this to implement the closest solution Sam was mentioning.

I'm not sure what comment by Sam you're referring to, but you don't have to fix that. In fact, since it's a separate problem it's better not to imo. You can do that in a separate pr in that case.

It's referenced here

Alright. I did read that, but too quickly I guess. I still think this should be a separate PR optimally. Solving it with closest also means 719p would count as HD, which it by definition isn't. I would expect it work like breakpoints, if anything. Sorry I'm making this hard for you btw!

friday · 2018-07-01T08:06:36Z

src/js/controls.js

                    type,
-                    title: controls.getLabel.call(this, 'quality', quality),
+                    title: controls.getLabel.call(this, 'quality', quality.height, toTitleCase(quality.label)),
                    badge: getBadge(quality),


I think we should respect the label and its formatting as it was specified by the developers, and only call getLabel if they didn't pass a label.

This includes i18n and changing the casing. In most languages I know of title case is incorrect formatting, and it make no sense to give developers the option to translate their own input. They should specify it in the language they want it.

friday · 2018-07-01T08:21:27Z

src/js/controls.js

    // Translate a value into a nice label
-    getLabel(setting, value) {
+    getLabel(setting, value, defaultLabel) {
+        let label;


I think you should restore getLabel (see above). Not sure why you removed ${value}p?

Is 272p or 408p legitimate? My understanding from your post on Slack was that the 'p' represents widescreen resolutions. Formally it's progressive scanning rather than interlaced.

No that was K As in 2K or 4K video etc (used for badges). 408p basically means its height is 408px (although the "p" does mean something else entirely).

Okay, replacing with the old logic for now.

friday · 2018-07-01T08:34:29Z

src/js/html5.js

-                return source && Number(source.getAttribute('size'));
+                // Return label, if match is found
+                return qualityLevels[sourceIndex].label;
            },


This should be height, not label

friday · 2018-07-01T08:39:26Z

src/js/listeners.js

            // Update UI
-            controls.updateSetting.call(this.player, 'quality', null, event.detail.quality);
+            controls.updateSetting.call(this.player, 'quality', null, event.detail.quality.label);
        });


This should be height, not label

friday · 2018-07-01T08:42:02Z

src/js/html5.js

                // Get first match for requested size
-                const source = sources.find(source => Number(source.getAttribute('size')) === input);
+                const source = sources.find(source => source.getAttribute('size') === input.label);



This should be height, not label

gurupras · 2018-07-01T10:04:27Z

No worries at all. I'd rather get it right than have others have to fix issues I introduce later.

…

On Sun, Jul 1, 2018 at 6:02 AM, friday ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/js/controls.js <#1081 (comment)>: > - if (!label.length) { - return null; + if (!label.length) { + // TODO: Try to find a badge based on height + return null; + } } Alright. I did read that, but too quickly I guess. I still think this should be a separate PR optimally. Solving it with closest also means 719p would count as HD, which it by definition isn't. I would expect it work like breakpoints, if anything. Sorry I'm making this hard for you btw! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1081 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACzB15oor-ZOB7wGv01wkseVVntYDtFBks5uCJ47gaJpZM4U-JyS> .

sampotts · 2018-07-01T10:15:26Z

Good point. It should be the second option then I guess. If larger or equal to 720 then it’s HD, larger than or equal to 1440 then it’s QHD etc. My point was I don’t think we need to force users to specify the labels. If they do it should override but if they don’t then we use sensible defaults to make life easy as it is now. Sam

…

On 1 Jul 2018, at 8:02 pm, friday ***@***.***> wrote: @friday commented on this pull request. In src/js/controls.js: > - if (!label.length) { - return null; + if (!label.length) { + // TODO: Try to find a badge based on height + return null; + } } Alright. I did read that, but too quickly I guess. I still think this should be a separate PR optimally. Solving it with closest also means 719p would count as HD, which it by definition isn't. I would expect it work like breakpoints, if anything. Sorry I'm making this hard for you btw! — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

Instead, index is taken as the array index and passed around internally where required

If unavailable, it is replaced with ${height}p

gurupras · 2018-07-02T14:48:43Z

I've incorporated most of the code-related feedback and made some fixes. Inviting more comments/feedback.

If required, I can add the default badge logic in this PR as well. I'm currently hesitant to make a separate PR for it as it would lead to some annoying merge conflicts if this PR were to be approved.

gurupras added 2 commits June 30, 2018 19:42

Tweaked quality selector to be more accomodating

14d0ce3

Added some minor changes to code relating to i18n

6bd9462

gurupras changed the base branch from master to develop July 1, 2018 00:53

sampotts reviewed Jul 1, 2018

View reviewed changes

gurupras added 2 commits June 30, 2018 20:56

Updated to use inbuilt type checkers

fda8424

Incorported early-return feedback

6e41d5e

gurupras force-pushed the better-quality-selector branch from 7ca36bb to 411d5b2 Compare July 1, 2018 01:16

gurupras commented Jul 1, 2018

View reviewed changes

Linting fixes

faaeb5d

gurupras force-pushed the better-quality-selector branch from 411d5b2 to faaeb5d Compare July 1, 2018 01:41

Fix plyr.quality setter

28add9b

friday reviewed Jul 1, 2018

View reviewed changes

gurupras added 6 commits July 1, 2018 19:36

Restored old ${value}p logic for displaying qualities

c3103f4

Removed index as a required property

99613ca

Instead, index is taken as the array index and passed around internally where required

Fix default quality

41fe611

Updated plyr.quality setter to continue to be backwards compatible

2e20972

Fixed dedupe

3643a03

Quality attribute label is no longer mandatory

b081ca0

If unavailable, it is replaced with ${height}p

Fixed getBadge logic

d50c65a

Uh oh!

Improve flexibility of quality selector #1081

Are you sure you want to change the base?

Improve flexibility of quality selector #1081

Uh oh!

Conversation

gurupras commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to related issue (if applicable)

Summary of proposed changes

Checklist

Uh oh!

gurupras commented Jul 1, 2018

Uh oh!

sampotts commented Jul 1, 2018

Uh oh!

gurupras commented Jul 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sampotts commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gurupras commented Jul 1, 2018

Uh oh!

friday commented Jul 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gurupras Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gurupras Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friday Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gurupras commented Jul 1, 2018 •

edited

Loading

sampotts commented Jul 1, 2018 •

edited

Loading

gurupras Jul 1, 2018 •

edited

Loading

gurupras Jul 1, 2018 •

edited

Loading

friday Jul 1, 2018 •

edited

Loading

friday Jul 1, 2018 •

edited

Loading