LIB-11: raw (low level) ISRC reads #6

JonnyJD · 2013-02-04T20:59:24Z

Most of the implementation for raw reading of CDs to get the Q sub-channels and the ISRCs in them is platform independent.
The only thing that is platform specific is scsi_cmd to issue a direct scsi command.

Raw reads are only optional in the spec (so I would keep other options as backup solution) and we might need to test for the support somehow.
Additionally: virtually all programs use the (more high level) "sub-channel command" approach. So that approach is tested and our "raw approch" would need more testing.

We also have to test and probably fix building again since we use new files and have new internal dependencies.

First tests show, that raw reading really fixes LIB-11

Sub-Tasks:

note to testers: There is a discisrc command you can use to test libdiscid without external tools.
For Windows testers there is a binary available: libdiscid-raw-win32.zip

JonnyJD · 2013-02-04T21:08:22Z

On Linux we used SG_IO, on Windows we probably should use IOCTL_SCSI_PASS_THROUGH_DIRECT.
No clue how it would work for Darwin, but it should work.

Like I said, implementing scsi_cmd for every platform is enough. We could als get MCNs on platforms where we don't have an easier solution. (But there seems to be no data quality problem with MCNs).

We read the raw data from the disc, extract the Q sub-channel and decode the ISRC. This gives better results (no duplicate ISRCs) than using the read ISRC sub-channel command. Probably due to bad implementation on some drives / drivers.

I actually tried SEEK, READ (6), SCAN and also PLAY AUDIO to move to some point more to the middle of the track in order not to get an ISRC from the previous track. None of these made any difference.

JonnyJD · 2013-02-07T16:14:26Z

I also tried read isrcs using the normal (READ SUB-CHANNEL) method, but trying multiple times.

That actually helped with some of the problems, but not all of the test cases.

JonnyJD · 2013-02-07T16:23:26Z

Not really sure why github shows this pull-request still having 6 commits. 4 of these are already in master as can also be seen in master...isrc_raw

JonnyJD · 2013-02-15T02:43:51Z

src/disc_scsi.c

+	/* there should be at least one ISRC in 100 sectors per spec
+	 * We try 150 (= 2 seconds) to be sure, but break when successfull
+	 */
+	const int SEC_NUM = 150;


We should make sure that these 150 sectors (= 2 seconds) are part of the track and check less sectors for VERY short tracks.

This should update the branch, especially add the features API and the compiler warning fixes. Conflicts: CMakeLists.txt configure.ac src/disc_linux.c

This is platform independent, except the call of scsi_cmd, which is implemented for the platform.

We don't have to prepend every file with "disc", even if they implement some type of disc reading.

Also separate scsi_cmd and scsi_cmd_unportable, although there is only unportable code right now. scsi_cmd_unportable not mandatory to implement for every platform, so it is in this header and not discid_private.h However, it is recommended to implement.

This should lower the chance of having a clash in the namespace. This is not as important as in the public API, but scsi.h is potentially included in many platform files.

After reading ISRCs the device keeps spinning, sometimes loudly. Stopping the device helps with that. This does have the side effect of stopping playback, when the disc is played directly. See: http://tickets.musicbrainz.org/browse/LIB-31

This merges the read_sparse functionality, lots of autotools changes, hiding of private functions in the library and adds version defines. LIBDISCID_INTERNAL was added to scsi.h manually. Conflicts: configure.ac src/Makefile.am src/disc_linux.c

The CRCs are not used yet, but I wanted to mark the location.

JonnyJD · 2013-04-26T17:19:48Z

Found some info about the CRC:
The CRC-16 is on CONTROL, ADR and DATA-Q. MSB is "first out". On disc parity bits are inverted, syndrome compared with 0, polynomial: x^16+x^12+x^5+1

That should be enough information to code something, but I didn't start, since I first have to look up how CRC actually works.

This merges the two windows platform files. Conflicts: CMakeLists.txt

This merges the data-track changes and removes Win9x code. Conflicts: Makefile.am src/disc_linux.c

This mainly merges the new test suite. Conflicts: Makefile.am

JonnyJD · 2013-06-30T00:14:33Z

Windows code is updated now, so scsi_cmd on Windows can be implemented.

I also added the test suite from master, making breakage more evident.

I implemented this from scratch. This is probably not the most efficient implementation, but the check doesn't even have a measurable impact. We are reading data from disc and only check the CRC when an ISRC is found, which is ca. every 100th frame. The focus is a clear implementation without storing (CRC-8) tables in the code. CRC mismatches are only printed to stderr for now.

Having memcheck in the branch would be nice.

Especially use defines for some magic numbers.

We read up to 3 ISRCs and use the first one where the CRC is correct. A warning is printed to stderr when CRC mismatches are found. We should probably skip the warning when these are very common.

phw · 2013-08-09T15:51:01Z

See pull request #39

JonnyJD · 2013-08-09T17:14:19Z

Unfortunately I didn't read much about scsi command availability detection yet. I remember reading something about that function being part of some MMC suite and that you could check if that suite is available somehow.
This possibly works together with the "sense data" returned for scsi commands, which isn't yet implemented on Windows (it works a bit weird there), but on Linux. (Have a look at scsi_cmd there).
Googling for "mmc_r10a" should yield a document about multimedia scsi commands. That is the spec the commands are defined in, but I am not sure if that was the spec were I read about feature detection.

However,
the tests show that we should be fine just checking for the case when no bytes are returned, which possibly means the command is not supported, but definately means we are not getting ISRCs (out of no data).

When no ISRCs are present (valid CRC or not) in the first 100 sectors (150 to be safe), there are no ISRCs on the track at all so we stop searching. Otherwise we would search the whole track, which then takes like 5 minutes per disc.

Reading more sectors takes more time. 10 more would be 10% more, rather than 50%. This should be fine.

JonnyJD · 2013-09-19T19:12:12Z

Discs without ISRCs should take like 10 seconds now, rather than 5 minutes before.
The spec says there should be one every 100 sectors (valid CRC or not) so we stop when there isn't one the first 110 sectors).

This was already fixed a month ago, but the unfixed commit was in the main repo.

This way we don't have to cast between HANDLE (void *) and int. We might need a third handle for OS X later on.

This adds raw isrcs reads on Windows. Conflicts: src/disc_linux.c src/scsi.h

JonnyJD · 2013-09-19T23:21:56Z

I merged the Windows code for raw ISRCs into this branch now.

@phw:
You had a drive that could find ISRCs on a disc with the normal method, but not the raw method.
Can you retest this now?
Relevant changes:
When no ISRCs (even invalid ones) are found in the first 110 sectors, then we don't search for more. So there shouldn't be a 5 minute read.
I also added some debug output in bc102db. Hopefull that will give some output indicating that something with the command itself isn't working (wrong return type, wrong amount of data returned, all data is zero).

phw · 2013-09-20T07:16:33Z

The code changs look good and work as expected. Now reading discs without ISRCs finishes fast.

But no news for the bad drive, it still finds no ISRCs with the raw method. Did you add anything new to the debug output? I didn't see anything and I still get the same result for every read:

scsi cmd bytes returned: 0
zero data returned by scsi_cmd

Not sure how to debug that further.

This hopefully enables us to catch some conditions when raw reading doesn't work.

This works, but actively checking for raw ISRC support might be better. There are error messages displayed for every track, when raw ISRC reads don't work.

JonnyJD · 2013-09-24T15:24:15Z

@phw:
We fall back to normal reads now when the scsi command doing raw ISRCs fails.
Please retest.

Currently warnings are printed (to stderr) for every track where raw ISRCs fail, since we don't actually know why it fails. The command can possibly fail for tracks due to weird offsets given in the TOC or other weird things.

The fallback is implemented in mb_scsi_read_track_isrc_raw() and falls back to mb_scsi_read_track_isrc(). So non-scsi isrc functions could be dropped. However, I don't want to do that until we have more testing done.

Some SCSI commands are optional per MMC so we check especially if raw ISRC reads are supported. It is still unclear if there are devices that can read audio CDs, but without CD READ support. Reported problems might be unrelated.

JonnyJD · 2013-09-24T20:15:42Z

I also implemented "real" scsi feature detection now.

@phw:
If the command for raw read really isn't available, then only one warning is shown and non-raw isrc reads are used.
However, from the specs I got the impression that any device supporting CDs should also support that command.
So possibly your problem is a different one. A bug in the driver?

Either way, I would like to know what the output is with your drive.
The detection, fall-back and check for zero length data returned are implemented for Linux and Windows. Possibly the results are different (if these are driver issues).

This way it corresponds with the command name in scsi.h The platform doesn't have to know how non-raw isrc reads work.

phw · 2013-09-25T07:24:56Z

src/disc_win32.c

 	}

 	for (i = disc->first_track_num; i <= disc->last_track_num; i++) {
 		if (features & DISCID_FEATURE_ISRC) {
-			read_disc_isrc(hDevice, disc, i);
+			scsi_features = mb_scsi_get_features(handle);


This should be done outside the loop

phw · 2013-09-25T07:35:26Z

That looks good, good work. Here is the debug output for my bad drive (in case you wonder, I have put the feature detection outside the for loop as I noted in the comments):

data requested, but none returned
Warning: could not fetch features
data requested, but none returned
Warning: could not fetch features
Warning: raw ISRCs not available, using ISRCs given by subchannel read
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!
WARNING: can't read subchannel data!

So for that drive it falls through to the old ISRC reading code. As expected my second drive prints no warnings and just works well with the raw SCSI code.

I have not yet tested the bad drive under Linux, I will do that later.

JonnyJD · 2013-10-01T23:46:30Z

Stalker-X (IRC) has the same warnings on windows:

data requested, but none returned
Warning: could not fetch features
Warning: raw ISRCs not available, using ISRCs given by subchannel read
WARNING: can't read subchannel data!data requested, but none returned
Warning: could not fetch features
WARNING: can't read subchannel data!data requested, but none returned
Warning: could not fetch features
WARNING: can't read subchannel data!data requested, but none returned
Warning: could not fetch features

(using libdiscid-w32-scsi.zip today)
(Plextor px-708a and px-716a)

The drive having problems for phw was MATSHITA DVD-RAM UJ892.

this pulls the TOC API and a lot of default drive changes Conflicts: src/disc.c src/disc_linux.c src/disc_win32.c

This should fix reading ISRCs on Windows again. (0.6.1) Conflicts: src/disc_win32.c

JonnyJD · 2013-10-04T11:08:40Z

I currently think the issues with some drives could be due to alignment problems.
I will try to implement gathering and using of the alignment of the adapter.

JonnyJD · 2013-10-04T15:15:12Z

The same drive that doesn't work for phw on windows, also doesn't work with raw ISRCs on Linux.
Scsi commands to work, the read reaw feature is detected, but no ISRCs can be found.
No warning is displayed, so the "no data returned" detection either doesn't trigger or doesn't work on Linux.

I should probably check if all returned data is 0 on linux or similar.

Zastai · 2016-05-20T19:15:31Z

Just adding this here: for the most recent MMC specs, google for mmc3r10g (MMC-3), mmc4r05a (MMC-4), mmc5r04 (MMC-5) and mmc6r02g (MMC-6). Later versions mainly add wording for bluray etc, not many actual changes. However, READ SUB-CHANNEL was actually dropped from the standard in MMC-5; it just refers you back to MMC-4. I'm not sure whether this is a case of "it's stable" or "it's only for cd's so just about obsolete", but it may mean that devices are no longer required to support them to comply with the MMC standard.

In my .NET implementation of libdiscid (https://github.com/zastai/MusicBrainz), I switched the Linux implementation to use SG_IO for all requests (i.e. also for the MCN and TOC) since this allowed reusing the MMC-based structures I used on Windows. I tried using SPTI on Windows (so I could share the CDB structures too), but couldn't get it to work; no great loss, and I'll probably try again when the rest of the platforms work.
BSD is up next; hopefully that will allow MMC-based operation too (SCIOCCOMMAND seems promising, although it requires write access to the device I think).

Side note 1: BSD (free/open/net) seem to have ioctls for subchannel data too (CDIOCREADSUBCHANNEL). I'll look into them and maybe submit a PR if I get it to work.

Side note 2: libdiscid's device detection for Net/OpenBSD cd devices is suboptimal; it should use getrawpartition() from libutil to find out which letter identifies the "whole disk" partition (in my case, this returns 2 ('c') on openbsd and 3 ('d') on netbsd) and then enumerate /dev/rcdnx based on that (with n between 0 and some maximum; 9 is probably ok). Systems with more than 2 cd devices are probably relatively rare (and in fact require explicitly running MAKEDEV to create the nodes for cd2 and up), but it would be good to support them. When I get the C# suff working, I'll look at updating libdiscid and submitting a PR.

JonnyJD added 2 commits February 4, 2013 22:24

read ISRCs raw on Linux

fbf133a

We read the raw data from the disc, extract the Q sub-channel and decode the ISRC. This gives better results (no duplicate ISRCs) than using the read ISRC sub-channel command. Probably due to bad implementation on some drives / drivers.

remove TODO comment

5fbf4d4

I actually tried SEEK, READ (6), SCAN and also PLAY AUDIO to move to some point more to the middle of the track in order not to get an ISRC from the previous track. None of these made any difference.

JonnyJD reviewed Feb 15, 2013
View reviewed changes

This was referenced Feb 16, 2013

discisrc/libdiscid backend (Mac/Win/Linux) JonnyJD/musicbrainz-isrcsubmit#35

Closed

removing backends in isrcsubmit2 JonnyJD/musicbrainz-isrcsubmit#48

Open

JonnyJD added 5 commits March 10, 2013 18:11

Merge branch 'master' into isrc_raw

c31d46b

This should update the branch, especially add the features API and the compiler warning fixes. Conflicts: CMakeLists.txt configure.ac src/disc_linux.c

move read_track_isrc to disc_scsi.c

f8a82dd

This is platform independent, except the call of scsi_cmd, which is implemented for the platform.

rename disc_scsi.c -> scsi.c

a1d73d6

We don't have to prepend every file with "disc", even if they implement some type of disc reading.

add HACKING file

d5a059b

ghost assigned JonnyJD Mar 11, 2013

JonnyJD added 6 commits March 11, 2013 01:48

prepend mb_scsi to scsi functions

b1616d3

This should lower the chance of having a clash in the namespace. This is not as important as in the public API, but scsi.h is potentially included in many platform files.

include info about SCSI standards

63c6dc8

linux: stop the device after reading

de09b17

After reading ISRCs the device keeps spinning, sometimes loudly. Stopping the device helps with that. This does have the side effect of stopping playback, when the disc is played directly. See: http://tickets.musicbrainz.org/browse/LIB-31

Merge branch 'master' into isrc_raw

1b859ac

This merges the read_sparse functionality, lots of autotools changes, hiding of private functions in the library and adds version defines. LIBDISCID_INTERNAL was added to scsi.h manually. Conflicts: configure.ac src/Makefile.am src/disc_linux.c

add scsi.h to distribution

1c013f8

locate CRC in q-channel

20b7fcb

The CRCs are not used yet, but I wanted to mark the location.

JonnyJD added 3 commits May 11, 2013 19:51

Merge branch 'master' into isrc_raw

81e062b

This merges the two windows platform files. Conflicts: CMakeLists.txt

Merge branch 'master' into isrc_raw

4df857c

This merges the data-track changes and removes Win9x code. Conflicts: Makefile.am src/disc_linux.c

Merge branch 'master' into isrc_raw

0618e64

This mainly merges the new test suite. Conflicts: Makefile.am

JonnyJD added 5 commits August 2, 2013 22:45

Merge branch 'master' into crc

5dd8ca2

Having memcheck in the branch would be nice.

improve comments and names for raw isrcs

867889f

Especially use defines for some magic numbers.

LIB-11: read ISRCs until CRC is valid

e77dd95

We read up to 3 ISRCs and use the first one where the CRC is correct. A warning is printed to stderr when CRC mismatches are found. We should probably skip the warning when these are very common.

free raw disc read data

b8d3ed3

JonnyJD added 2 commits September 19, 2013 19:57

check if there are ISRCs at all

4a4ebab

When no ISRCs are present (valid CRC or not) in the first 100 sectors (150 to be safe), there are no ISRCs on the track at all so we stop searching. Otherwise we would search the whole track, which then takes like 5 minutes per disc.

define SECTORS_ISRC and read 10 more

73e1fa6

Reading more sectors takes more time. 10 more would be 10% more, rather than 50%. This should be fine.

JonnyJD added 4 commits September 20, 2013 00:14

check for data_len, rather than 96

ee22ed7

This was already fixed a month ago, but the unfixed commit was in the main repo.

create mb_scsi_handle struct

aed1d9f

This way we don't have to cast between HANDLE (void *) and int. We might need a third handle for OS X later on.

Merge branch 'scsi_handle' into scsi_windows

e34d987

Merge branch 'scsi_windows' into isrc_raw

e2fc17f

This adds raw isrcs reads on Windows. Conflicts: src/disc_linux.c src/scsi.h

JonnyJD added 3 commits September 24, 2013 16:36

return usable error codes from scsi_cmd

2803ca0

This hopefully enables us to catch some conditions when raw reading doesn't work.

fix warnings on MinGW

bdd74be

fall back to normal when raw ISRC fails

046d1c0

This works, but actively checking for raw ISRC support might be better. There are error messages displayed for every track, when raw ISRC reads don't work.

implement scsi feature detection

cd58b5d

Some SCSI commands are optional per MMC so we check especially if raw ISRC reads are supported. It is still unclear if there are devices that can read audio CDs, but without CD READ support. Reported problems might be unrelated.

rename subchannel feature to isrc

7031c1b

This way it corresponds with the command name in scsi.h The platform doesn't have to know how non-raw isrc reads work.

phw reviewed Sep 25, 2013
View reviewed changes

windows: restructure isrc section

55d274f

JonnyJD added 4 commits October 2, 2013 01:50

add newline to stderr message

3923e5a

Merge branch 'master' into isrc_raw

6533105

this pulls the TOC API and a lot of default drive changes Conflicts: src/disc.c src/disc_linux.c src/disc_win32.c

Merge branch 'master' into isrc_raw

90dbfe5

This should fix reading ISRCs on Windows again. (0.6.1) Conflicts: src/disc_win32.c

Merge branch 'master' into isrc_raw

477da28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LIB-11: raw (low level) ISRC reads #6

LIB-11: raw (low level) ISRC reads #6

JonnyJD commented Feb 4, 2013

JonnyJD commented Feb 4, 2013

JonnyJD commented Feb 7, 2013

JonnyJD commented Feb 7, 2013

JonnyJD Feb 15, 2013

JonnyJD commented Apr 26, 2013

JonnyJD commented Jun 30, 2013

phw commented Aug 9, 2013

JonnyJD commented Aug 9, 2013

JonnyJD commented Sep 19, 2013

JonnyJD commented Sep 19, 2013

phw commented Sep 20, 2013

JonnyJD commented Sep 24, 2013

JonnyJD commented Sep 24, 2013

phw Sep 25, 2013

phw commented Sep 25, 2013

JonnyJD commented Oct 1, 2013

JonnyJD commented Oct 4, 2013

JonnyJD commented Oct 4, 2013

Zastai commented May 20, 2016

LIB-11: raw (low level) ISRC reads #6

Are you sure you want to change the base?

LIB-11: raw (low level) ISRC reads #6

Conversation

JonnyJD commented Feb 4, 2013

JonnyJD commented Feb 4, 2013

JonnyJD commented Feb 7, 2013

JonnyJD commented Feb 7, 2013

JonnyJD Feb 15, 2013

Choose a reason for hiding this comment

JonnyJD commented Apr 26, 2013

JonnyJD commented Jun 30, 2013

phw commented Aug 9, 2013

JonnyJD commented Aug 9, 2013

JonnyJD commented Sep 19, 2013

JonnyJD commented Sep 19, 2013

phw commented Sep 20, 2013

JonnyJD commented Sep 24, 2013

JonnyJD commented Sep 24, 2013

phw Sep 25, 2013

Choose a reason for hiding this comment

phw commented Sep 25, 2013

JonnyJD commented Oct 1, 2013

JonnyJD commented Oct 4, 2013

JonnyJD commented Oct 4, 2013

Zastai commented May 20, 2016