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

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

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 47 commits into from

Conversation

JonnyJD
Copy link
Contributor

@JonnyJD JonnyJD commented Feb 4, 2013

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:

  • platform independent part
  • Linux
  • Windows
  • OS X / Darwin
  • Check for Command availability
  • Check CRCs

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
Copy link
Contributor Author

JonnyJD commented Feb 4, 2013

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
Copy link
Contributor Author

JonnyJD commented Feb 7, 2013

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
Copy link
Contributor Author

JonnyJD commented Feb 7, 2013

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

/* 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@ghost ghost assigned JonnyJD Mar 11, 2013
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
Copy link
Contributor Author

JonnyJD commented Apr 26, 2013

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
Copy link
Contributor Author

JonnyJD commented Jun 30, 2013

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
Copy link
Member

phw commented Aug 9, 2013

See pull request #39

@JonnyJD
Copy link
Contributor Author

JonnyJD commented Aug 9, 2013

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
Copy link
Contributor Author

JonnyJD commented Sep 19, 2013

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
Copy link
Contributor Author

JonnyJD commented Sep 19, 2013

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
Copy link
Member

phw commented Sep 20, 2013

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
Copy link
Contributor Author

JonnyJD commented Sep 24, 2013

@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
Copy link
Contributor Author

JonnyJD commented Sep 24, 2013

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.
}

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done outside the loop

@phw
Copy link
Member

phw commented Sep 25, 2013

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
Copy link
Contributor Author

JonnyJD commented Oct 1, 2013

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
Copy link
Contributor Author

JonnyJD commented Oct 4, 2013

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
Copy link
Contributor Author

JonnyJD commented Oct 4, 2013

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
Copy link
Contributor

Zastai commented May 20, 2016

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.

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

Successfully merging this pull request may close these issues.

3 participants