superg / redumper

Low level CD dumper utility
GNU General Public License v3.0
200 stars 18 forks source link

PX-4824 doesn't jump multisession gap #171

Closed HeroponRikiBestest closed 2 weeks ago

HeroponRikiBestest commented 2 months ago

I want to add the necessary code to https://github.com/superg/redumper/blob/99ab2005036d8c32682af6b624b9cf79218140c1/cd/cd_dump.ixx#L715 for it to function properly for the relevant drives, but I want to make sure that whatever change I propose would be acceptable. What's the issue with the currently commented-out FIXME, and is there any approach you would accept in its place at the moment?

(To be clear, I understand that active development is still paused; this is not an urgent ask, and I'm just posting this issue for when active development does eventually resume)

superg commented 2 months ago

Can you elaborate, what's the issue here? Doesn't redumper function as intended on "slow sector drives"?

HeroponRikiBestest commented 2 months ago

No, it fails to jump the session gap; once it gets there, it just gets really slow to the point of basically hitting a wall, since the FIXME part for slow sectors isn't implemented yet, and since the drive never throws any SCSI errors. If you cancel the dump once it hits that section and then refine, it jumps the gap on refine and finishes the dump without issue. It would just be nice if it was handled for those drives during the initial dump too.

superg commented 2 months ago

You're making false assumptions about my // FIXME: label, it's there because I don't like that negated if() condition, commented out code is not needed at all. As far as I understand you have a set up that compiles so try to experiment setting SLOW_SECTOR_TIMEOUT to something else and see if you get it to jump over gap.

HeroponRikiBestest commented 2 months ago

As is, the slow variable is already getting set properly. To be clear, I have confirmed the slow variable is being set properly via debugging/print statements.

As far as I can tell, the slow variable just doesn't do anything, since the slow variable makes it hit that empty if statement every read, rather than the else if for errors or else for normal reads.

HeroponRikiBestest commented 2 months ago

Went and double-checked. It doesn't hit that if statement since it's negated like you said; it just keeps doing normal reads, but never jumps since the drive never returns any errors since it's just doing progressively slower and slower reads, and the slow variable is only used on that if statement, which isn't hit anyways.

superg commented 1 month ago

I think you misunderstand that if() statement. The current code logic is "if drive is plextor, we spent 5 seconds or more and we're reading the sector inside gap - jump over the gap". e.g. to jump the gap (what you want) you need to get into that empty if() block. 5 seconds might be too long for your drive and I suggested to reduce that number and see it if jumps for you.

HeroponRikiBestest commented 1 month ago

Gotcha, I'll take another look. Just wondering, why does the check include seeing if it's a Plextor? Wouldn't you want to skip the gap on ASUS and Generic drives too, if they have a read that slow in the sector gap?

superg commented 1 month ago

That's because all other drives return scsi error when accessing multisession gaps. It's just plextors who have it unlocked (but have trouble reading it fast because no positional Q in 2nd session TOC).

HeroponRikiBestest commented 1 month ago

Ah, I see the issue; I'm using a 4824 specifically, which I didn't realize until now has configuration type generic, I imagine because if a drive has the plextor configuration, it'll try to read full leadin as well, which the 4824 obv can't do.

Also, I don't think I've ever experienced that for my ASUS? My asus will read through most if not all of the session gap if you let it, just somewhat slowly (checking redumper's own calculations; while the speed of each slow sector read is about 1-2 seconds and varies depending on drive speed, the first slow sector is almost always exactly 5 seconds)

superg commented 1 month ago

I'll repeat, other drives return SCSI error when accessing the gap. Those errors are fast so it just flies over the gap. The jump is needed only for drives that don't return an error but get super slow accessing those areas.

4824 is an exception though, maybe we want to add it to this check. Feel free to add the check in a same manner as it was done in CD-TEXT and I will approve PR.

HeroponRikiBestest commented 1 month ago

I'll repeat, this does not happen on my ASUS. I am watching the sector count with verbose output the entire time. It does not skip any. I remember sometimes it would skip at least part of the gap, but only towards the very end, and I can't get that to happen at the moment anyways, so that could be a false recollection. If you do not believe me, I can send a recording of a dump if it would help.

Regarding the 4824, sounds good, I can add it to the check.

superg commented 1 month ago

I'll repeat, this does not happen on my ASUS.

This is what I'm saying, each sector in the gap is read on ASUS and as error is returned instantly, there is no need to jump. Is that clear?

superg commented 1 month ago

No need for an option, I mean if(drive type is plextor or vendor is plextor and model is 4824), just how it's checked there based on vendor/model string.

HeroponRikiBestest commented 1 month ago

I'll repeat, this does not happen on my ASUS.

This is what I'm saying, each sector in the gap is read on ASUS and as error is returned instantly, there is no need to jump. Is that clear?

No; double-checking to be more specifc, it seems that the last 1500-ish sectors are not returned instantly, and take 1-2 seconds each to read, so a session gap can take 10-20 minutes to get through on 8x speed. Regardless, I won't push it further, and I apologize for bothering you.

Regarding the 4824, sounds good, I'll PR as soon as I put that in.

superg commented 1 month ago

No; double-checking to be more specifc, it seems that the last 1500-ish sectors are not returned instantly, and take 1-2 seconds each to read, so a session gap can take 10-20 minutes to get through on 8x speed. Regardless, I won't push it further, and I apologize for bothering you.

Let me guess, you're using patched 3.10 Rib's firmware? :)

HeroponRikiBestest commented 1 month ago

Yes, I use Rib's firmware. Whatever the issue is, I'm sorry, never mind.

Anyhow, opened the PR for the PX-4824 check. Please request any changes as necessary, and thank you for your assistance thus far.

superg commented 1 month ago

So, while Rib's firmware unlocks leadout ranges for all sessions, there is another issue where ASUS firmware is doing more processing if encountered sector is leadout. It's not even TOC based but when sector has AA track number, there is some delay and I guess more things can be patched there. TL;DR, ASUS leadout reading is slower but it's not slow enough to jump the gap because what is returned is a very valid sector. Otherwise, none of this happen on the 99% of the drives where leadout is just locked, it literally flies through the gap.

HeroponRikiBestest commented 1 month ago

Makes sense.

HeroponRikiBestest commented 1 month ago

Sorry, forgot to fix the title yesterday.

Anyhow, would it be okay for me to open an issue for the rib firmware thing, and attempt to put together a PR for that too, separate to the current PR I already submitted?

Something like, adding a commandline option to not try and read the whole multisession gap for rib firwmare users. With the internal logic being, when that option is passed, do a check for if the drive configuration is ribshark (or just asus, if ribshark can't specifically be checked, since you have to choose to pass that commandline option anyways) and if the drive is within 1500 sectors of the end of the error range, jump the gap?

If not, fair enough. Just wanted to make sure I explored all my options there.

superg commented 1 month ago

I don't consider it a problem as you just have to wait a little bit longer for the gap - you still get legit sectors. The percentage of multisession disc vs single session discs is minimal, I don't think there is an issue at all.

HeroponRikiBestest commented 1 month ago

"A little bit longer" is 20 minutes added for just that part of the gap, which is why I consider it a problem that you can't at least specify a commandline option to jump it. Regardless, if you don't think it's an issue, fair enough. Apologies for pestering you about it for this many replies, and let me know once you've had a chance to look at the PR.

superg commented 1 month ago

Yes, 20 minutes is longer, but it's not 2+ hours. Think about it this way - I don't like making firmware workarounds in redumper unless absolutely neccessary. In this case I believe Rib could patch those additional checks for leadout sectors and it will be faster.

HeroponRikiBestest commented 3 weeks ago

Sorry to poke, but I just wanted to ask if there was anything my PR https://github.com/superg/redumper/pull/172 still needed. I understand that you're busy, but I was hoping the PR could be merged at some point just due to being a short and simple change. If you have too much going on for that atm, I understand.

superg commented 3 weeks ago

I'll try to check on a weekend.

HeroponRikiBestest commented 3 weeks ago

Thank you! Sorry about that