Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rr-get-output-info fix #198

Closed
einsiedlerspiel opened this issue Jan 11, 2023 · 1 comment
Closed

rr-get-output-info fix #198

einsiedlerspiel opened this issue Jan 11, 2023 · 1 comment

Comments

@einsiedlerspiel
Copy link
Contributor

einsiedlerspiel commented Jan 11, 2023

The index crtc-start in rr-get-output-info is 26 when it should be 36, resulting in mangled output for everything that depends on that index.

clx/extensions/randr.lisp

Lines 665 to 693 in e8c9033

(defun rr-get-output-info (display output config-timestamp &key (result-type 'list))
"FIXME: indexes might be off, name not decoded properly"
(with-buffer-request-and-reply (display (randr-opcode display) nil :sizes (8 16 32))
((data +rr-getoutputinfo+)
(card32 output)
(card32 config-timestamp))
(let* ((num-crtcs (card16-get 26))
(num-modes (card16-get 28))
(num-clones (card16-get 32))
(name-length (card16-get 34))
(crtc-start 26)
(mode-start (index+ crtc-start (index* num-crtcs 4)))
(clone-start (index+ mode-start (index* num-modes 4)))
(name-start (index+ clone-start (index* num-clones 4))))
(values
(member8-vector-get 1 +rr-config-status+)
(card32-get 8) ; timestamp
(card32-get 12) ; current connected crtc
(card32-get 16) ; width in mm
(card32-get 20) ; height in mm
(member8-vector-get 24 +rr-connection+)
(member8-vector-get 25 +render-subpixel-order+) ; sub-pixel-order
(sequence-get :result-type result-type :length num-crtcs :index 26)
(card16-get 30)
(sequence-get :result-type result-type :length num-modes :index mode-start)
(sequence-get :result-type result-type :length num-clones :index clone-start)
;(string-get name-length name-start )
(sequence-get :result-type 'string :format card16 :length name-length :index name-start :transform #'code-char))
)))

I'd love to have a fixed version of it. Both #172 and the merged and now reversed #193 provide that. Before I open a third pull request on this i wanted to check in what the preferred fix would look like for this function.

The easiest fix is to just change the crtc-start index to 36 and change the line that gets the value back to the string-get-version (switch line 692 for 691).

The slightly more extensive fix would be to also reorder the output values to be compliant with the randr spec. Though reordering the output values might again break existing software (though I'm questioning whether anyone is using this function in its current state anyway).

I do like the version of the function implemented by #193 for being more readable, though it does reorder the output values in a way that still doesn't match the spec.

clx/extensions/randr.lisp

Lines 752 to 808 in a85ad99

(defun get-output-info (display output config-timestamp
&key (result-type 'list))
"Return information for the output OUTPUT on DISPLAY.
OUTPUT is the id of an output.
CONFIG-TIMESTAMP is the ?.
Return 11 values:
1. the status of OUTPUT
2. the timestamp ?
3. the name of OUTPUT as a string
4. the id of the CRTC currently using OUTPUT
5. the physical width of OUTPUT in millimeters
6. the physical height of OUTPUT in millimeters
7. the connection state of OUTPUT
8. the sub-pixel order of OUTPUT
9. a sequence of type RESULT-TYPE of ids of CRTCs which could use
OUTPUT(?)
10. a sequence of type RESULT-TYPE of ids of modes which OUTPUT could
use(?)
11. a sequence of type RESULT-TYPE of ids of ?"
(declare (type display display)
(type output-id output))
(with-buffer-request-and-reply (display (randr-opcode display) nil
:sizes (8 16 32))
((data +rr-getoutputinfo+)
(card32 output)
(card32 config-timestamp))
(let* ((status (member8-vector-get 1 +config-status+))
(timestamp (card32-get 8))
(current-crtc (card32-get 12))
(width-in-mm (card32-get 16))
(height-in-mm (card32-get 20))
(connection (member8-vector-get 24 +connection+))
(sub-pixel-order (member8-vector-get 25 +render-subpixel-order+))
(crtcs-num (card16-get 26))
(modes-num (card16-get 28))
(clones-num (card16-get 32))
(name-length (card16-get 34))
(crtcs-start 36)
(crtcs (sequence-get :result-type result-type
:length crtcs-num
:index crtcs-start))
(modes-start (index+ crtcs-start (index* crtcs-num 4)))
(modes (sequence-get :result-type result-type
:length modes-num
:index modes-start))
(clones-start (index+ modes-start (index* modes-num 4)))
(clones (sequence-get :result-type result-type
:length clones-num
:index clones-start))
(name-start (index+ clones-start (index* clones-num 4)))
(name (string-get name-length name-start)))
(values status timestamp name current-crtc
width-in-mm height-in-mm connection sub-pixel-order
crtcs modes clones))))

What should be done?

@einsiedlerspiel
Copy link
Contributor Author

StumpWM depends on the order of return values.

https://github.com/stumpwm/stumpwm/blob/9b5e0cfd19d0dfb18e284ac140431a2685afbe3e/head.lisp#L41-L60

And just in general changing them is probably a bad idea at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant