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.

Sign up for GitHub

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

Jump to bottom

Bug 1000784 #26

Merged
merged 8 commits into from
Jul 3, 2013
Merged

Bug 1000784 #26

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c63b6d1
Added verbosity (filenames) on TagLib errors.
troyane Jun 25, 2013
a1e7bb4
Formated code according to Mixxx' Coding Guidelines
troyane Jun 25, 2013
1a1b4cb
Specified filenames on errors.
troyane Jun 26, 2013
8365956
Added spaces around the ==
troyane Jun 26, 2013
feacb68
Added spaces around ==
troyane Jun 26, 2013
41d1ea1
merged w/ upstream
troyane Jul 2, 2013
d12fd10
Improved type mismatch errors.
troyane Jul 2, 2013
9f710b4
Move all warnings from the bottom of each parseHeader() to
troyane Jul 3, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 14 additions & 19 deletions src/soundsourcecoreaudio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,8 @@ int SoundSourceCoreAudio::open() {
err = ExtAudioFileOpen(&fsRef, &m_audioFile);
*/

if (err != noErr)
{
qDebug() << "SSCA: Error opening file.";
if (err != noErr) {
qDebug() << "SSCA: Error opening file " << m_qFilename;
return ERR;
}

Expand All @@ -72,9 +71,8 @@ int SoundSourceCoreAudio::open() {
UInt32 size = sizeof(inputFormat);
m_inputFormat = inputFormat;
err = ExtAudioFileGetProperty(m_audioFile, kExtAudioFileProperty_FileDataFormat, &size, &inputFormat);
if (err != noErr)
{
qDebug() << "SSCA: Error getting file format";
if (err != noErr) {
qDebug() << "SSCA: Error getting file format (" << m_qFilename << ")";
return ERR;
}

Expand Down Expand Up @@ -119,9 +117,8 @@ int SoundSourceCoreAudio::open() {
size = sizeof(clientFormat);

err = ExtAudioFileSetProperty(m_audioFile, kExtAudioFileProperty_ClientDataFormat, size, &clientFormat);
if (err != noErr)
{
qDebug() << "SSCA: Error setting file property";
if (err != noErr) {
qDebug() << "SSCA: Error setting file property (" << m_qFilename << ")";
return ERR;
}

Expand All @@ -133,9 +130,8 @@ int SoundSourceCoreAudio::open() {
SInt64 totalFrameCount;
dataSize = sizeof(totalFrameCount); //XXX: This looks sketchy to me - Albert
err = ExtAudioFileGetProperty(m_audioFile, kExtAudioFileProperty_FileLengthFrames, &dataSize, &totalFrameCount);
if (err != noErr)
{
qDebug() << "SSCA: Error getting number of frames";
if (err != noErr) {
qDebug() << "SSCA: Error getting number of frames (" << m_qFilename << ")";
return ERR;
}

Expand Down Expand Up @@ -185,9 +181,8 @@ long SoundSourceCoreAudio::seek(long filepos) {
//qDebug() << "SSCA: Seeking to" << segmentStart;

//err = ExtAudioFileSeek(m_audioFile, filepos / 2);
if (err != noErr)
{
qDebug() << "SSCA: Error seeking to" << filepos;// << GetMacOSStatusErrorString(err) << GetMacOSStatusCommentString(err);
if (err != noErr) {
qDebug() << "SSCA: Error seeking to" << filepos << " (file " << m_qFilename << ")";// << GetMacOSStatusErrorString(err) << GetMacOSStatusCommentString(err);
}
return filepos;
}
Expand Down Expand Up @@ -270,10 +265,10 @@ int SoundSourceCoreAudio::parseHeader() {
// Feels like 1995 again...
}


if (result)
return OK;
return ERR;
if (result == ERR) {
Copy link
Member

Choose a reason for hiding this comment

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

and here ...

Copy link
Member

Choose a reason for hiding this comment

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

I think we have a consistency problem above:

  // Takes care of all the default metadata
        result = processTaglibFile(f);

        // Now look for MP3 specific metadata (e.g. BPM)
        TagLib::ID3v2::Tag* id3v2 = f.ID3v2Tag();
        if (id3v2) {
            processID3v2Tag(id3v2);
        }

"result" is not checked before the additional Taglib call but the whole function returns error in this case.
IMHO there are two logical paths: Abort when one call fails, or graceful recover errors and not return ERR if some call succeed.

TagLib::AudioProperties *properties = f.audioProperties();

processTaglibFile fails if f.audioProperties() returns NULL

@rryan: What is the right strategy? I would prefer the early abort. Since this is Legacy code, I would prefer fixing it in a separate branch. There is also my external Tagreader process (slowly) on the way.

qWarning() << "Error parsing header of file" << m_qFilename;
}
return result ? OK : ERR;
}


Expand Down
5 changes: 3 additions & 2 deletions src/soundsourceffmpeg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,8 @@ long SoundSourceFFmpeg::seek(long filepos)
ret = av_seek_frame(pFormatCtx, audioStream, fspos, AVSEEK_FLAG_BACKWARD /*AVSEEK_FLAG_ANY*/);

if (ret){
qDebug() << "ffmpeg: Seek ERROR ret(" << ret << ") filepos(" << filepos << "d).";
qDebug() << "ffmpeg: Seek ERROR ret(" << ret << ") filepos(" << filepos << "d) at file"
<< m_qFilename;
unlock();
return 0;
}
Expand Down Expand Up @@ -379,7 +380,7 @@ int SoundSourceFFmpeg::ParseHeader( TrackInfoObject * Track )

fname = location.toAscii();
FFmpegInit();
qDebug() << "ffmpeg: pqrsing file:" << fname;
qDebug() << "ffmpeg: parsing file:" << fname;
if(av_open_input_file(&FmtCtx, fname.constData(), NULL, 0, NULL)!=0)
{
qDebug() << "av_open_input_file: cannot open" << fname;
Expand Down
17 changes: 13 additions & 4 deletions src/soundsourceflac.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ SoundSourceFLAC::~SoundSourceFLAC() {
// soundsource overrides
int SoundSourceFLAC::open() {
m_file.open(QIODevice::ReadOnly);

m_decoder = FLAC__stream_decoder_new();
if (m_decoder == NULL) {
qWarning() << "SSFLAC: decoder allocation failed!";
Expand Down Expand Up @@ -90,6 +91,8 @@ int SoundSourceFLAC::open() {
FLAC__stream_decoder_finish(m_decoder);
FLAC__stream_decoder_delete(m_decoder);
m_decoder = NULL;

qWarning() << "SSFLAC: Decoder error at file" << m_qFilename;
return ERR;
}

Expand All @@ -99,7 +102,9 @@ long SoundSourceFLAC::seek(long filepos) {
// but libflac expects a number in time samples. I _think_ this should
// be hard-coded at two because *2 is the assumption the caller makes
// -- bkgood
FLAC__stream_decoder_seek_absolute(m_decoder, filepos / 2);
bool result = FLAC__stream_decoder_seek_absolute(m_decoder, filepos / 2);
if (!result)
qWarning() << "SSFLAC: Seeking error at file" << m_qFilename;
m_leftoverBufferLength = 0; // clear internal buffer since we moved
return filepos;
}
Expand All @@ -115,7 +120,7 @@ unsigned int SoundSourceFLAC::read(unsigned long size, const SAMPLE *destination
if (m_flacBufferLength == 0) {
i = 0;
if (!FLAC__stream_decoder_process_single(m_decoder)) {
qWarning() << "SSFLAC: decoder_process_single returned false";
qWarning() << "SSFLAC: decoder_process_single returned false (" << m_qFilename << ")";
break;
} else if (m_flacBufferLength == 0) {
// EOF
Expand Down Expand Up @@ -168,6 +173,9 @@ int SoundSourceFLAC::parseHeader() {
if (xiph) {
processXiphComment(xiph);
}
if (result == ERR) {
qWarning() << "Error parsing header of file" << m_qFilename;
Copy link
Member

Choose a reason for hiding this comment

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

and here ..

}
return result ? OK : ERR;
}

Expand All @@ -194,7 +202,7 @@ inline FLAC__int16 SoundSourceFLAC::shift(FLAC__int32 sample) const {
} else {
return sample << shift;
}
};
}

// static
QList<QString> SoundSourceFLAC::supportedFileExtensions() {
Expand All @@ -220,6 +228,7 @@ FLAC__StreamDecoderSeekStatus SoundSourceFLAC::flacSeek(FLAC__uint64 offset) {
if (m_file.seek(offset)) {
return FLAC__STREAM_DECODER_SEEK_STATUS_OK;
} else {
qWarning() << "SSFLAC: An unrecoverable error occurred (" << m_qFilename << ")";
return FLAC__STREAM_DECODER_SEEK_STATUS_ERROR;
}
}
Expand Down Expand Up @@ -308,7 +317,7 @@ void SoundSourceFLAC::flacError(FLAC__StreamDecoderErrorStatus status) {
break;
}
qWarning() << "SSFLAC got error" << error << "from libFLAC for file"
<< m_file.fileName();
<< m_qFilename;
// not much else to do here... whatever function that initiated whatever
// decoder method resulted in this error will return an error, and the caller
// will bail. libFLAC docs say to not close the decoder here -- bkgood
Expand Down
4 changes: 4 additions & 0 deletions src/soundsourcemodplug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ SoundSourceModPlug::SoundSourceModPlug(QString qFilename) :
modFile.close();
// get ModPlugFile descriptor for later access
m_pModFile = ModPlug::ModPlug_Load(m_fileBuf.data(), m_fileBuf.length());

if (m_pModFile==NULL) {
qDebug() << "[ModPlug] Error while ModPlug_Load";
}
}

SoundSourceModPlug::~SoundSourceModPlug()
Expand Down
18 changes: 11 additions & 7 deletions src/soundsourcemp3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ int SoundSourceMp3::open()

// This is not a working MP3 file.
if (currentframe == 0) {
qDebug() << "SSMP3: This is not a working MP3 file:"
<< m_qFilename;
return ERR;
}

Expand Down Expand Up @@ -204,6 +206,7 @@ long SoundSourceMp3::seek(long filepos) {
}

if (!isValid()) {
qDebug() << "SSMP3: Error wile seeking file " << m_qFilename;
return 0;
}

Expand Down Expand Up @@ -298,7 +301,7 @@ long SoundSourceMp3::seek(long filepos) {
}
}

// Synthesize the the samples from the frame which should be discard to reach the requested position
// Synthesize the samples from the frame which should be discard to reach the requested position
if (cur != NULL) //the "if" prevents crashes on bad files.
discard(filepos-cur->pos);
}
Expand Down Expand Up @@ -347,7 +350,6 @@ long SoundSourceMp3::seek(long filepos) {
// Unfortunately we don't know the exact fileposition. The returned position is thus an
// approximation only:
return filepos;

}

inline long unsigned SoundSourceMp3::length() {
Expand Down Expand Up @@ -432,8 +434,10 @@ unsigned long SoundSourceMp3::discard(unsigned long samples_wanted) {
*/
unsigned SoundSourceMp3::read(unsigned long samples_wanted, const SAMPLE * _destination)
{
if (!isValid())
if (!isValid()) {
qDebug() << "SSMP3: Error while reading " << m_qFilename;
return 0;
}

// Ensure that we are reading an even number of samples. Otherwise this function may
// go into an infinite loop
Expand Down Expand Up @@ -476,7 +480,6 @@ unsigned SoundSourceMp3::read(unsigned long samples_wanted, const SAMPLE * _dest
rest = -1;
return Total_samples_decoded;
}

}

// qDebug() << "Decoding";
Expand Down Expand Up @@ -587,9 +590,10 @@ int SoundSourceMp3::parseHeader()
processAPETag(ape);
}

if (result)
return OK;
return ERR;
if (result == ERR) {
qWarning() << "Error parsing header of file" << m_qFilename;
Copy link
Member

Choose a reason for hiding this comment

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

here is a type mismatch: const int ERR = -1; bool result

}
return result ? OK : ERR;
}

int SoundSourceMp3::findFrame(int pos)
Expand Down
11 changes: 6 additions & 5 deletions src/soundsourceoggvorbis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ int SoundSourceOggVorbis::open()
{
if (ret == OV_EINVAL) {
//The file is not seekable. Not sure if any action is needed.
qDebug() << "oggvorbis: file is not seekable " << m_qFilename;
}
}

return OK;

}

/*
Expand Down Expand Up @@ -146,7 +146,7 @@ long SoundSourceOggVorbis::seek(long filepos)
// frames and we pretend to the world that everything is stereo)
return ov_pcm_tell(&vf) * 2;
} else{
qDebug() << "ogg vorbis: Seek ERR.";
qDebug() << "ogg vorbis: Seek ERR at file " << m_qFilename;
return 0;
}
}
Expand Down Expand Up @@ -257,9 +257,10 @@ int SoundSourceOggVorbis::parseHeader() {
processXiphComment(tag);
}

if (result)
return OK;
return ERR;
if (result == ERR) {
qWarning() << "Error parsing header of file" << m_qFilename;
Copy link
Member

Choose a reason for hiding this comment

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

The same her here I think: const int ERR = -1; bool result

}
return result ? OK : ERR;
}

/*
Expand Down
8 changes: 5 additions & 3 deletions src/soundsourcesndfile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ unsigned SoundSourceSndFile::read(unsigned long size, const SAMPLE * destination
}

// The file has errors or is not open. Tell the truth and return 0.
qDebug() << "The file has errors or is not open: " << m_qFilename;
return 0;
}

Expand Down Expand Up @@ -228,9 +229,10 @@ int SoundSourceSndFile::parseHeader()
}
}

if (result)
return OK;
return ERR;
if (result == ERR) {
qWarning() << "Error parsing header of file" << m_qFilename;
Copy link
Member

Choose a reason for hiding this comment

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

and here: const int ERR = -1; bool result

}
return result ? OK : ERR;
}

/*
Expand Down