Crozzers / screen_brightness_control

A Python tool for controlling the brightness of your monitor
https://crozzers.github.io/screen_brightness_control
MIT License
50 stars 9 forks source link

Fix `allow_duplicates` behavior in `set_brightness()` for relative string values #37

Closed BingoWon closed 8 months ago

BingoWon commented 8 months ago

This PR addresses a bug related to the allow_duplicates in the set_brightness() when the value is a relative string value (e.g., "+20" or "-10").

During the process of adding pytest cases for this scenario, I noticed a discrepancy in the definition of "duplicates". In the current implementation, it is supposed that duplicates could occur in two scenarios:

  1. The same monitor is connected through different interfaces at the same time.
  2. Different monitors share identical identifiers, including EDID, due to careless manufacturer.

In both scenarios, the list_monitors_info() function returns the same output: different indices but identical other fields. This behavior was observed on a Windows 11 system.

[{'name': 'None Generic Monitor', 'model': 'Generic Monitor', 'serial': '', 'manufacturer': None, 'manufacturer_id': 'IPS', 'edid': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc', 'method': screen_brightness_control.screen_brightness_control.windows.VCP, 'index': 0}, {'name': 'None Generic Monitor', 'model': 'Generic Monitor', 'serial': '', 'manufacturer': None, 'manufacturer_id': 'IPS', 'edid': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc', 'method': screen_brightness_control.screen_brightness_control.windows.VCP, 'index': 1}]

However, in the mock modules (helpers_mock.py and os_module_mock.py), the first and third displays share the same index but have many different other fields.

[{ 'name': 'Brand Display1', 'model': 'Display1', 'manufacturer': 'Brand', 'manufacturer_id': 'BRD', 'serial': 'serial1', 'edid': '00ffffffffff00edid1', 'method': cls, 'index': 0 }] [{ 'name': 'Brand Display3', 'model': 'Display3', 'manufacturer': 'Brand', 'manufacturer_id': 'BRD', 'serial': 'serial3', 'edid': '00ffffffffff00edid3', 'method': cls, 'index': 0 }]

Do you suggest any modifications around allow_duplicates?

Crozzers commented 8 months ago

the first and third displays share the same index

The index is not "global" but is scoped to the OS specific method that controls that monitor. The test displays are configured to represent different sets of monitors that can be controlled in different ways. In the real world, this might look like this:

# first laptop display detected
sbc.windows.WMI.get_brightness(display=0)
# first desktop monitor detected. Same index but definitely not the same as WMI[0]
sbc.windows.VCP.get_brightness(display=0)

This means that using the index as the identifier inside set_brightness will cause issues:

import screen_brightness_control as sbc
monitors = [
  {'method': sbc.windows.WMI, 'index': 0, 'name': 'Display A'},
  {'method': sbc.windows.VCP, 'index': 0, 'name': 'Display B'},
]
for monitor in sbc.filter_monitors(haystack=monitors):
  # will call with `display=0` twice, only setting the laptop display
  sbc.set_brightness(100, display=monitor['index'])

We could either use the Display class to get around this, or we could add the method kwarg to our set_brightness call.

for monitor in sbc.filter_monitors(haystack=monitors):
  # option 1:
  display_instance = Display.from_dict(monitor)
  # calls the OS method directly with the method-scoped index
  display.set_brightness(100)

  # option 2
  # includes all the error handling from __brightness
  set_brightness(100, display=monitor['index'], method=monitor['method'])

Option 1 is probably more efficient but I don't really mind which way it goes

Crozzers commented 8 months ago

I'm also starting to wonder if, for your monitors, there may be more information in the EDID extension blocks. The following code should get the full edid for each of your monitors:

import screen_brightness_control as sbc
import struct
from pprint import pprint

wmi = sbc.windows._wmi_init()
edids = {}
for monitor in wmi.WmiMonitorDescriptorMethods():
    base = ''.join(f'{char:02x}' for char in monitor.WmiGetMonitorRawEEdidV1Block(0)[0])
    following_blocks = struct.unpack(sbc.helpers.EDID.EDID_FORMAT, bytes.fromhex(base))[-2]
    blocks = [base] + [
        ''.join(f'{char:02x}' for char in monitor.WmiGetMonitorRawEEdidV1Block(i)[0])
        for i in range(1, following_blocks + 1)
    ]
    edids[monitor.InstanceName] = ''.join(blocks)

pprint(edids)
print('All EDIDs identical:', len(set(edids.values())) == 1)

If they're not all identical, it may be worth figuring out how to add parsing for EDID extension blocks. I haven't up until now since most displays seem to store all the identifying info in the first edid block

BingoWon commented 8 months ago

I'm also starting to wonder if, for your monitors, there may be more information in the EDID extension blocks. The following code should get the full edid for each of your monitors:

import screen_brightness_control as sbc
import struct
from pprint import pprint

wmi = sbc.windows._wmi_init()
edids = {}
for monitor in wmi.WmiMonitorDescriptorMethods():
    base = ''.join(f'{char:02x}' for char in monitor.WmiGetMonitorRawEEdidV1Block(0)[0])
    following_blocks = struct.unpack(sbc.helpers.EDID.EDID_FORMAT, bytes.fromhex(base))[-2]
    blocks = [base] + [
        ''.join(f'{char:02x}' for char in monitor.WmiGetMonitorRawEEdidV1Block(i)[0])
        for i in range(1, following_blocks + 1)
    ]
    edids[monitor.InstanceName] = ''.join(blocks)

pprint(edids)
print('All EDIDs identical:', len(set(edids.values())) == 1)

If they're not all identical, it may be worth figuring out how to add parsing for EDID extension blocks. I haven't up until now since most displays seem to store all the identifying info in the first edid block

{'DISPLAY\\IPS2800\\5&21344eb6&0&UID4355_0': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc02032af24f610102030410131f292f3f4060101fe200c023097f0783010000e305c000e6060501626200023a801871382d40582c25006b5c2100001aab5e00a0a0a02d50302045006b5c2100001a56bd70a080a02d50302045006b5c2100001a34e300a0a0a02d50302045006b5c2100009a00000000000000000000000000d370123f000003003c11f90184ff0e030157002b006f081d0002000f004b7a0104ff0e2f02af0057006f08280002000f00cfa00104ff0ec7002f001f006f08280002000f00a50000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000090',
 'DISPLAY\\IPS2800\\5&21344eb6&0&UID4357_0': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc02032af24f610102030410131f292f3f4060101fe200c023097f0783010000e305c000e6060501626200023a801871382d40582c25006b5c2100001aab5e00a0a0a02d50302045006b5c2100001a56bd70a080a02d50302045006b5c2100001a34e300a0a0a02d50302045006b5c2100009a00000000000000000000000000d370123f000003003c11f90184ff0e030157002b006f081d0002000f004b7a0104ff0e2f02af0057006f08280002000f00cfa00104ff0ec7002f001f006f08280002000f00a50000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000090'}
All EDIDs identical: True
Crozzers commented 8 months ago

LGTM. Thanks for this.