capLevelToPlayerSize and devicePixelRatio

[i8beef]

What do you want to do with Hls.js?

I would like to use capLevelToPlayerSize, but with out the devicePixelRatio adjustment introduced here:

hls.js/src/controller/cap-level-controller.ts

Line 225 in 2896a73

static get contentScaleFactor(): number {

I was looking at using this setting to pick lower resolution streams between small in-line players and full screen mode... unfortunately, because so many devices use a devicePixelRatio with higher resolution displays now, its stuck always picking the highest stream because it's taking the video element's dimensions, and then multiplying it by 2 or more.

Essentially, I'd like this mode to respect the specified dimensions of the video element to choose streams and not try and adjust.

What have you tried so far?

Various forms of live patching the objects themselves, but I haven't found a combination that lets me override contentScaleFactor to always return 1. Up for any suggestions, but I'm unsure how I could do this without an added config option to effect this directly...

[i8beef]

I forked and added a setting like I'm suggesting. If you like this approach, I am happy to go through a PR for this. Thoughts?

[robwalch]

Using devicePixelRatio is intentional. When using capLevelToPlayerSize hls.js will limit ABR to the variant with the highest resolution that can be rendered within the video element's current dimensions without a lot of downsampling. This accounts for retina and mobile screens that can display higher resolutions than are reported by JavaScript in "pixels".

You can always set autoLevelCapping to the highest level of your choice if you know better. This actually disables the interval testing of your video element's bounds (Yay! DOM rendering performance 🤗). If you know the width and height you want to limit playback to, cycle through hls.levels to find the best fit, and set hls.autoLevelCapping to that level's index.

[i8beef]

I realize it's intentional, but forcing it into pulling streams that are exponentially more expensive to stream (both client performance and bandwidth wise) than respecting the static dimensions set by the page author seems heavy handed... I won't argue the need, of course, but I'd like to request the OPTION to ignore devicePixelRatio in these calculations so that we can control this otherwise static, and inaccessible, setting that has major implications for stream performance.

I actually wouldn't mind expanding my change to a more general setting to allow ignoring devicePixelRatio everywhere (I think I only saw one other place) as I imagine turning that off in one place would indicate the user's desire to turn it off EVERYWHERE, so all calculations relative to it are consistent... would that be more palatable? Something like a shared getDevicePixelRatio somewhere that is setting aware for all usages of this?

[i8beef]

Something like this? master...i8beef:master

This also gives you a nice disconnect point from a direct window dependency, and allows external callers to even see how the displayPixelRatio is determined internally if needed...

[phillipseamore]

Just a note, this would make more sense as enableDevicePixelRatio instead of ignoreDevicePixelRatio and defaulting to true.

Then the devicePixelRatio function can also be simpler:

  get devicePixelRatio(): number {
    let pixelRatio = 1;

    if(this.config.enableDevicePixelRatio) {    
	    try {
	      pixelRatio = self.devicePixelRatio;
	    } catch (e) {
	      /* no-op */
	    }
	}

    return pixelRatio;
  }

Also when seeing this (in contentScaleFactor), is the try/catch necessary? Wouldn't pixelRatio = self.devicePixelRatio || 1 suffice?

[i8beef]

I am open to any requested changes to get this in, I'm flexible! I just went ignore because it seemed "retina support" was the desired DEFAULT, and I tend not to make optional settings in stuff that default to a value different from the underlying type: in this case, boolean defaults to false, so I went with something that would default the same way... but I could just as easily force it to default true and go the other way. Note, my PERSONAL opinion is to go that way as I think "retina support" should be opt in instead of opt out, but I may be biased 😉

I kept the try catch as it was used in the other implementation... I assumed it had something to do with being used in a non-browser / test scenario where window may be undefined.

[phillipseamore]

Respecting DPI is the desired default as that is in-line with how other media elements should be used (img with srcset and picture with source for example). The quality difference is quite noticeable. As an example CSS in Safari on an iPhone 11 will tell you that it's 414x896 pixels when the display really is 828x1792 (and it will render everything at 2x and upscale images and videos 2x if they don't have pixels to fill the display).

[i8beef]

I think that's an excellent point... I'd point out though getting around this exact situation is why the img srcset supported the x descriptor.

[i8beef]

I've looked through your contribution guidelines and made all changes I think are necessary to my fork. Should I make a PR, or is there a process to moving from "feature proposal" to a PR?