Rework device profile building to be more accurate #744
Conversation
GodTamIt
force-pushed
the
profiles
branch
3 times, most recently
from
0b03f63 to
eaf5dd8
Compare
|
So much better! |
GodTamIt
force-pushed
the
profiles
branch
3 times, most recently
from
ec949df to
c151aea
Compare
This addresses several issues with how device profile building was done:
1. Devices were hard-coded, which did not account for the many
different device profiles that may not be 1st party Google
Chromecast™️ devices. This includes almost every Android TV with
Cast functionality built-in.
2. Different devices may have slightly different profile requirements,
so even if an overall codec WAS correct, certain profiles may not
be supported.
3. Resolution constraints were being sent as a poor representation of
of video codec profile and bit depth constraints. This change fixes
this issue.
4. This also improves the accuracy of the codec detection for less
popular codecs like AV1.
| const codecString = getCodecString(codec, profile, level, bitDepth); | ||
|
|
||
| // Limit the upper bound to 32K, which is more than enough. | ||
| while (maxRes.width < 30720) { |
There was a problem hiding this comment.
Technically, 32K is not 30720, it's 32768... so either the comment above is wrong or the constant here is wrong. Perhaps this 30720 is actually meant to be a some H * W calculation, in which case, showing that calc might be better?
There was a problem hiding this comment.
I'm not sure this is correct. Here are the common 16:9 resolutions:
- HD: 1920 × 1080
- 4K: 3840 × 2160
- 8K: 7680 × 4320
- 16K: 15360 × 8640
- 32K: 30720 × 17280 (speculated, not common)
There was a problem hiding this comment.
That's where that number comes from, gotcha... old (16-bit) programmer brain. :)
Perhaps make the comment
// Limit the upper bound to 32K resolution (30720 × 17280), which is more than enough.
There was a problem hiding this comment.
(you are correct that 30720 is a calculation though. it's 1920 * 16, where 1920 is the width component of 1920 × 1080)
| const possibleProfiles = ((): string[] => { | ||
| switch (codec) { | ||
| case VideoCodec.H264: | ||
| return [ |
There was a problem hiding this comment.
These constants and the ones in getCodecString are very tightly coupled (as is the logic of formatting the string coupled to the base VideoCodec). Would it be better to actually have a derived class instance for each VideoCodec variant that knows how to format it's own codec string, and exposes the underlying profiles supported?
There was a problem hiding this comment.
I think I understand what you mean, but do you mind providing a mini example?
FYI, this coupling starts to weaken with an upcoming change I'm stacking on top of this that adds support for checking BT.2020 and SMPTE ST 2084 support.
| ): number | undefined { | ||
| const possibleLevels = ((): number[] => { | ||
| switch (codec) { | ||
| case VideoCodec.H264: |
There was a problem hiding this comment.
Similar comment to this, would a specific instance of a derived class for each VideoCodec, that exposed the levels supported as a property be clearer?
| const maxBitDepth = maxBitDepths[0]; | ||
| const maxResolution = maxResolutions[0]; | ||
|
|
||
| const profileConditions = [ |
There was a problem hiding this comment.
You didn't change the logic from before, but this condition seems odd... why do we care if it is anamorphic?
There was a problem hiding this comment.
I don't know enough about what videos are anamorphic anymore (iiuc, it's an artifact of 35mm film -> old CRT 4:3 home displays), so I didn't really change it here. If you have some context as to why we won't care about this anymore, more than happy to remove it.
These checks aren't too expensive, as they're mainly just calling into the underlying Chromium engine. Chromium internally should do a lot of the caching here. Because some parts of the output compatibility matrix could, theoretically, be dynamic, I was hesitant to bifurcate the logic into cacheable and non-cacheable profiles. I guess you're right that video tends to be static but I guess resolution isn't always (if you mess around in settings). Take audio output, for example. It's not uncommon for people to plug and unplug soundbars and/or for them to turn off and on. I do this all the time because of a bug in mine lol. |
This addresses several issues with how device profile building was done:
Issues: