[WIP] Add read speed option - #60
Conversation
Bloomca
left a comment
There was a problem hiding this comment.
I think this API looks good! I left a few comments for the implementation, also there a few typos in the comments but we can fix it later.
I think we should an example of "slow_read_track" which would read the first track from the default drive at 10x.
@strict-flower One question I am curious about and forgot to ask in the issue. If custom multiplier does persist (we'll need to test it), do you think we should set speed back to optimal in the drive destructor?
| /// By MMC-3 specification, It can be set to the optimal speed of the drive when it executes SET CD SPEED command with read speed (KB/s) = 0xFFFF. | ||
| /// On the Linux, the read speed will selected by the CDROM_SELECT_SPEED ioctl with speed = 0. It'll set the speed automatically and highest speed that supported by the drive. | ||
| Optimal, | ||
| /// Use the custom speed with specified multiplier. |
There was a problem hiding this comment.
We should add some inline documentation that it is 176KB/s * multiplier. Also maybe give an example that 10x is usually slow enough.
There was a problem hiding this comment.
I tried to add that. Is this what you had in mind?
| #include <IOKit/storage/IOCDMediaBSDClient.h> | ||
|
|
||
| Boolean request_cd_read_speed(int fd, uint16_t target_speed_kbs) { | ||
| int ret = ioctl(fd, DKIOCCDSETSPEED, target_speed_kbs); |
There was a problem hiding this comment.
I think we need to pass a pointer there (I remember it from your example in the issue and here it dereferences
There was a problem hiding this comment.
just typo. I've fixed that.
| let target_speed_kbs = if multiplier == 0 { | ||
| 0xffff | ||
| } else { | ||
| multiplier * 176400 / 1000 |
There was a problem hiding this comment.
This can overflow. With u16, it is not that hard to do, however, generally seems that audio CD can max out at ~52.
So maybe we can clamp the multiplier to 100x?
There was a problem hiding this comment.
I've changed CustomMultiplier(u16) to CustomMultiplier(u8) since u8 is sufficient to it I think. This change will fix it.
| multiplier * 176400 / 1000 | ||
| }; | ||
| if target_speed_kbs > u16::MAX.into() { | ||
| // TODO: Implement error handle (CdReaderError?) |
There was a problem hiding this comment.
Similar to before, I think it might be easier to clamp the multiplier and this way we will never overflow
|
@Bloomca Thanks for the quick review. I've fixed them. I'll write & testing the Linux implementation today.
I tested that. The DKIOCCDSETSPEED ioctl seems to set the drive speed until the disc is ejected. Below is my experiment summary.
... and the default state seems to be Optimal, but we can't confirm that because DKIOCCDGETSPEED only returns the current speed1, according to my experiments. Footnotes
|
|
I think it's difficult to restore the original state. At least, macOS doesn't seem to provide a way to get the original state. Moreover, we could get the read speed performance data of the drive, but we can't get the current speed policy of the drive. In other words, even if the current speed is 10x, we can't determine whether the value was set by another program, is the default value of the drive, or was selected automatically.
|
|
I wrote the implementation for Linux and tested. It seems to work correctly. Also I tested
Although I didn't eject the CD during the above procedure, the speed setting did not persist. On Linux, it seems to be reset on each Additionally, I tested this on an internal disc drive (ASUS BC-12D2HT). This drive seems to ignore the requested speed with the Thus, unlike my earlier comments, I think the best destructor behaviour may be OS-dependent. However, there is also the possibility that we don't need to restore the policy at all.
Footnotes
|
Under development
Description
Background discussion is #59
Adds an option to request to set the read speed of the tracks.