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

CachingReader: Reduce log spam for corrupt files #652

Merged
merged 27 commits into from
Jul 28, 2015
Merged

CachingReader: Reduce log spam for corrupt files #652

merged 27 commits into from
Jul 28, 2015

Conversation

uklotzde
Copy link
Contributor

Partially fixes (decoding errors at the end of a file): https://bugs.launchpad.net/mixxx/+bug/1470010

  • Shorten readable length of corrupt files to avoid repeated decoding
  • Code cleanups: Consistent naming, remove unncessary checks and local variables, ...

// forward it makes sense to keep in memory, the hinter can provide
// either 0 for a forward hint or -1 for a backward hint. We should
// be calculating an appropriate number of samples to go backward as
// some function of the latency, but for now just leave this as a
Copy link
Member

Choose a reason for hiding this comment

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

some time ago we have changed the wording from "latency" to "audio buffer".
Did you mean here: "as some function of the selected audio buffer in hardware preferences?"

@daschuer
Copy link
Member

I can confirm that the log spam is reduced playing the faulty files from
https://github.com/jrmuizel/AudioScan-opus

However now I have files that won't end. They are stucked just before the and with schakin waveforms
https://github.com/jrmuizel/AudioScan-opus/blob/master/t/mp3/v2.3-iso-8859-1.mp3
This does not happen without the patch.

This file also prevent AutoDJ form continuing.

@uklotzde
Copy link
Contributor Author

Thanks for finding another edge case, Daniel 👍

@uklotzde
Copy link
Contributor Author

My last commit should fix this. Now the requested read buffer is simply
filled with zeros if sample data is unreadable. This applies to both the
beginning ("preroll") as well as the end of the file.

On 07/20/2015 05:09 PM, Daniel Schürmann wrote:

I can confirm that the log spam is reduced playing the faulty files from
https://github.com/jrmuizel/AudioScan-opus

However now I have files that won't end. They are stucked just before the
and with schakin waveforms
https://github.com/jrmuizel/AudioScan-opus/blob/master/t/mp3/v2.3-iso-8859-1.mp3
This does not happen without the patch.

This file also prevent AutoDJ form continuing.


Reply to this email directly or view it on GitHub
#652 (comment).

@daschuer
Copy link
Member

Issue fixed.

However I found crasher:
https://github.com/jrmuizel/AudioScan-opus/blob/master/t/mp4/array-keys-int.m4a
ReadAtom: "/home/daniel/workspace/t/libaudio-scan-perl-master/t/mp4/array-keys-int.m4a": invalid atom size, extends outside parent atom - skipping to end of "" "mdat" 5848240 vs 135015
GetTrackMediaDataName: "/home/daniel/workspace/t/libaudio-scan-perl-master/t/mp4/array-keys-int.m4a": track 1 has more than 1 child atoms in stsd

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffb89fd700 (LWP 30058)]
__strcasecmp_l_ssse3 () at ../sysdeps/x86_64/multiarch/../strcmp.S:209
209 ../sysdeps/x86_64/multiarch/../strcmp.S: Datei oder Verzeichnis nicht gefunden.
(gdb) bt
#0  __strcasecmp_l_ssse3 () at ../sysdeps/x86_64/multiarch/../strcmp.S:209
#1  0x00007fffd1b1c14c in findFirstAudioTrackId (hFile=0x7fffb4002100)
    at plugins/soundsourcem4a/soundsourcem4a.cpp:72
#2  Mixxx::SoundSourceM4A::tryOpen (this=0x7fffb4003ca0, audioSrcCfg=...)
    at plugins/soundsourcem4a/soundsourcem4a.cpp:123
#3  0x0000000000ac646e in Mixxx::SoundSource::open (this=0x7fffb4003ca0, 
    audioSrcCfg=...) at src/sources/soundsource.cpp:28
#4  0x0000000000ac4865 in SoundSourceProxy::openAudioSource (
    this=this@entry=0x7fffb89fcd00, audioSrcCfg=...)
    at src/soundsourceproxy.cpp:152
#5  0x00000000004de864 in openAudioSourceForReading (audioSrcCfg=..., 
    pTrack=...) at src/cachingreaderworker.cpp:167
#6  CachingReaderWorker::loadTrack (this=this@entry=0x1e6fb18, pTrack=...)
    at src/cachingreaderworker.cpp:199
#7  0x00000000004dfb2b in CachingReaderWorker::run (this=0x1e6fb18)
    at src/cachingreaderworker.cpp:150
#8  0x00007ffff540032f in ?? () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
#9  0x00007ffff31e6182 in start_thread (arg=0x7fffb89fd700)
    at pthread_create.c:312
#10 0x00007ffff189247d in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
(gdb) 

@uklotzde
Copy link
Contributor Author

We have already fixed this one some time ago. The current master (just did a
clean build) doesn't crash on this file. I get an appropriate error log and
Mixxx refuses to load the file:

Debug [Main]: WCoverArt::slotCoverFound WCoverArt(0x4bee4d0)
"CoverInfo(METADATA,GUESSED,,48529,/home/uk/Music/Corrupt Tracks
(Link)/array-keys-int.m4a)" QSize(300, 300)
Read: "/home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a": stsd
inconsistency with number of entries
ReadAtom: "/home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a": invalid
atom size, extends outside parent atom - skipping to end of "" "mdat"
5848240 vs 135015
GetTrackMediaDataName: "/home/uk/Music/Corrupt Tracks
(Link)/array-keys-int.m4a": track 1 has more than 1 child atoms in stsd
Warning [CachingReaderWorker 1]: No AAC track found:
"file:///home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a"
Warning [CachingReaderWorker 1]: Failed to open SoundSource
Warning [CachingReaderWorker 1]: Failed to open file:
"/home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a"
Debug [CachingReaderWorker 1]: "[Channel1]" CachingReaderWorker::loadTrack()
load failed for" "/home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a"
", file invalid, unlocked reader lock
Debug [Main]: Failed to load track "/home/uk/Music/Corrupt Tracks
(Link)/array-keys-int.m4a" "The file '/home/uk/Music/Corrupt Tracks
(Link)/array-keys-int.m4a' could not be loaded."

On 07/21/2015 08:47 AM, Daniel Schürmann wrote:

Issue fixed.

However I found crasher:
https://github.com/jrmuizel/AudioScan-opus/blob/master/t/mp4/array-keys-int.m4a
ReadAtom:
"/home/daniel/workspace/t/libaudio-scan-perl-master/t/mp4/array-keys-int.m4a":
invalid atom size, extends outside parent atom - skipping to end of ""
"mdat" 5848240 vs 135015
GetTrackMediaDataName:
"/home/daniel/workspace/t/libaudio-scan-perl-master/t/mp4/array-keys-int.m4a":
track 1 has more than 1 child atoms in stsd

|Program received signal SIGSEGV, Segmentation fault. [Switching to Thread
0x7fffb89fd700 (LWP 30058)] __strcasecmp_l_ssse3 () at
../sysdeps/x86_64/multiarch/../strcmp.S:209 209
../sysdeps/x86_64/multiarch/../strcmp.S: Datei oder Verzeichnis nicht
gefunden. (gdb) bt #0 __strcasecmp_l_ssse3 () at
../sysdeps/x86_64/multiarch/../strcmp.S:209 #1 0x00007fffd1b1c14c in
findFirstAudioTrackId (hFile=0x7fffb4002100) at
plugins/soundsourcem4a/soundsourcem4a.cpp:72 #2
Mixxx::SoundSourceM4A::tryOpen (this=0x7fffb4003ca0, audioSrcCfg=...) at
plugins/soundsourcem4a/soundsourcem4a.cpp:123 #3 0x0000000000ac646e in
Mixxx::SoundSource::open (this=0x7fffb4003ca0, audioSrcCfg=...) at
src/sources/soundsource.cpp:28 #4 0x0000000000ac4865 in
SoundSourceProxy::openAudioSource ( this=this@entry=0x7fffb89fcd00,
audioSrcCfg=...) at src/soundsourceproxy.cpp:152 #5 0x00000000004de864 in
openAudioSourceForReading (audioSrcCfg=..., pTrack=...) at
src/cachingreaderworker.cpp:167 #6 CachingReaderWorker::loadTrack
(this=this@entry=0x1e6fb18, pTrack=...) at src/cachingreaderworker.cpp:199
#7 0x00000000004dfb2b in CachingReaderWorker::run (this=0x1e6fb18) at
src/cachingreaderworker.cpp:150 #8 0x00007ffff540032f in ?? () from
/usr/lib/x86_64-linux-gnu/libQtCore.so.4 #9 0x00007ffff31e6182 in
start_thread (arg=0x7fffb89fd700) at pthread_create.c:312 #10
0x00007ffff189247d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111 (gdb) |


Reply to this email directly or view it on GitHub
#652 (comment).

@uklotzde
Copy link
Contributor Author

2015-06-30 Merge pull request #626 from
uklotzde/CachingReaderAvoidUnnecessarySeeking

Fix outstanding SoundSource issues

On 07/21/2015 11:54 AM, Uwe Klotz wrote:

We have already fixed this one some time ago. The current master (just did
a clean build) doesn't crash on this file. I get an appropriate error log
and Mixxx refuses to load the file:

Debug [Main]: WCoverArt::slotCoverFound WCoverArt(0x4bee4d0)
"CoverInfo(METADATA,GUESSED,,48529,/home/uk/Music/Corrupt Tracks
(Link)/array-keys-int.m4a)" QSize(300, 300)
Read: "/home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a": stsd
inconsistency with number of entries
ReadAtom: "/home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a":
invalid atom size, extends outside parent atom - skipping to end of ""
"mdat" 5848240 vs 135015
GetTrackMediaDataName: "/home/uk/Music/Corrupt Tracks
(Link)/array-keys-int.m4a": track 1 has more than 1 child atoms in stsd
Warning [CachingReaderWorker 1]: No AAC track found:
"file:///home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a"
Warning [CachingReaderWorker 1]: Failed to open SoundSource
Warning [CachingReaderWorker 1]: Failed to open file:
"/home/uk/Music/Corrupt Tracks (Link)/array-keys-int.m4a"
Debug [CachingReaderWorker 1]: "[Channel1]"
CachingReaderWorker::loadTrack() load failed for" "/home/uk/Music/Corrupt
Tracks (Link)/array-keys-int.m4a" ", file invalid, unlocked reader lock
Debug [Main]: Failed to load track "/home/uk/Music/Corrupt Tracks
(Link)/array-keys-int.m4a" "The file '/home/uk/Music/Corrupt Tracks
(Link)/array-keys-int.m4a' could not be loaded."

On 07/21/2015 08:47 AM, Daniel Schürmann wrote:

Issue fixed.

However I found crasher:
https://github.com/jrmuizel/AudioScan-opus/blob/master/t/mp4/array-keys-int.m4a
ReadAtom:
"/home/daniel/workspace/t/libaudio-scan-perl-master/t/mp4/array-keys-int.m4a":
invalid atom size, extends outside parent atom - skipping to end of ""
"mdat" 5848240 vs 135015
GetTrackMediaDataName:
"/home/daniel/workspace/t/libaudio-scan-perl-master/t/mp4/array-keys-int.m4a":
track 1 has more than 1 child atoms in stsd

|Program received signal SIGSEGV, Segmentation fault. [Switching to Thread
0x7fffb89fd700 (LWP 30058)] __strcasecmp_l_ssse3 () at
../sysdeps/x86_64/multiarch/../strcmp.S:209 209
../sysdeps/x86_64/multiarch/../strcmp.S: Datei oder Verzeichnis nicht
gefunden. (gdb) bt #0 __strcasecmp_l_ssse3 () at
../sysdeps/x86_64/multiarch/../strcmp.S:209 #1 0x00007fffd1b1c14c in
findFirstAudioTrackId (hFile=0x7fffb4002100) at
plugins/soundsourcem4a/soundsourcem4a.cpp:72 #2
Mixxx::SoundSourceM4A::tryOpen (this=0x7fffb4003ca0, audioSrcCfg=...) at
plugins/soundsourcem4a/soundsourcem4a.cpp:123 #3 0x0000000000ac646e in
Mixxx::SoundSource::open (this=0x7fffb4003ca0, audioSrcCfg=...) at
src/sources/soundsource.cpp:28 #4 0x0000000000ac4865 in
SoundSourceProxy::openAudioSource ( this=this@entry=0x7fffb89fcd00,
audioSrcCfg=...) at src/soundsourceproxy.cpp:152 #5 0x00000000004de864 in
openAudioSourceForReading (audioSrcCfg=..., pTrack=...) at
src/cachingreaderworker.cpp:167 #6 CachingReaderWorker::loadTrack
(this=this@entry=0x1e6fb18, pTrack=...) at
src/cachingreaderworker.cpp:199 #7 0x00000000004dfb2b in
CachingReaderWorker::run (this=0x1e6fb18) at
src/cachingreaderworker.cpp:150 #8 0x00007ffff540032f in ?? () from
/usr/lib/x86_64-linux-gnu/libQtCore.so.4 #9 0x00007ffff31e6182 in
start_thread (arg=0x7fffb89fd700) at pthread_create.c:312 #10
0x00007ffff189247d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111 (gdb) |


Reply to this email directly or view it on GitHub
#652 (comment).

@daschuer
Copy link
Member

My test result was generated by this PR merged to the current master.
So #626 should be part of it.
Maybe the issue is slightly different or I have messed up my repository.

@daschuer
Copy link
Member

Ah, sorry for the false alarm, I have missed to use the recent m4a plugin from gdb.
Now it works.

...to trigger a Travis CI rebuild after a strange test failure ;)
@uklotzde
Copy link
Contributor Author

There is a bug in the preroll calculation that causes segmentation faults during the tests. Fix will follow.


SINT frameIndex = samples2frames(sample);
SINT numFrames = samples2frames(numSamples);

// TODO: is it possible to move this code out of caching reader
// and into enginebuffer? It doesn't quite make sense here, although
// it makes preroll completely transparent to the rest of the code
// if we're in preroll...
Copy link
Member

Choose a reason for hiding this comment

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

It is not the first time I am struggling with this comment. I like the current solution, because it simplifies the code elsewhere. If you think the same, we should remove this comment.

DEBUG_ASSERT(nullptr == m_pPrev);

m_pNext = pBefore;
if (pBefore) {
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 this can also be an assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To insert an element at the head of the queue you need to pass a pointer to
the head pHead == pBefore. If the list is currently empty there is no head
and consequently pHead == pBefore == nullptr. If we add an assertion the
caller would be responsible to check for emptiness before calling this
function. I prefer to handle the special case of an empty list inside this
function.

On 07/27/2015 01:49 PM, Daniel Schürmann wrote:

In src/cachingreaderchunk.cpp
#652 (comment):

  • m_state = READY;
    +}

+void CachingReaderChunk::free() {

  • DEBUG_ASSERT(READ_PENDING != m_state);
  • CachingReaderChunkWorker::init(kDefaultIndex);
  • m_state = FREE;
    +}

+void CachingReaderChunk::insertIntoListBefore(

  •    CachingReaderChunk\* pBefore) {
    
  • DEBUG_ASSERT(nullptr == m_pNext);
  • DEBUG_ASSERT(nullptr == m_pPrev);
  • m_pNext = pBefore;
  • if (pBefore) {

I think this can also be an assertion.


Reply to this email directly or view it on GitHub
https://github.com/mixxxdj/mixxx/pull/652/files#r35527619.

}

// The state is controlled by the cache as the owner of each chunk!
void giveToWorker() {
Copy link
Member

Choose a reason for hiding this comment

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

Nice! :-)

@daschuer
Copy link
Member

Sorry, the CachingReaderChunk split still not works for me.

For example: CachingReaderChunkForWorker::indexForFrame is accessed from cachingReader.

In the current state it looks more confusing than helpful to me.

A better solution may look like this, using two friend Keys:
http://stackoverflow.com/questions/1609472/friend-class-with-limited-access

Since the surrounding code is (thanks to all the refactoring and comments) pretty clear now,
there is IMHO no need for such a split or any extra effort.
So what about renaming just CachingReaderChunkForWorker to CachingReaderChunkBase change the comments accordingly and merge this PR?

By the way: The rest looks good to me. Thank you very much.

@uklotzde
Copy link
Contributor Author

I didn't take the static functions and constants in the base class into
account while renaming. Of course, that looks confusing ;)

I will split the chunk into 3 classes:

  • CachingReaderChunk
  • CachingReaderChunkForOwner (i.e. the cache)
  • CachingReaderChunkForWorker (i.e. the worker thread)

On 07/28/2015 10:27 AM, Daniel Schürmann wrote:

Sorry, the CachingReaderChunk split still not works for me.

For example: CachingReaderChunkForWorker::indexForFrame is accessed from
cachingReader.

In the current state it looks more confusing than helpful to me.

A better solution may look like this, using two friend Keys:
http://stackoverflow.com/questions/1609472/friend-class-with-limited-access

Since the surrounding code is (thanks to all the refactoring and comments)
pretty clear now,
there is IMHO no need for such a split or any extra effort.
So what about renaming just CachingReaderChunkForWorker to
CachingReaderChunkBase change the comments accordingly and merge this PR?

By the way: The rest looks good to me. Thank you very much.


Reply to this email directly or view it on GitHub
#652 (comment).

@uklotzde
Copy link
Contributor Author

Ups, no, 3 classes don't work without further complications. The following
two classes are a viable solution, both comprehensive and effective:

  • CachingReaderChunk (base class for both the owner and the worker)
  • CachingReaderChunkForOwner (i.e. the cache)

On 07/28/2015 10:45 AM, Uwe Klotz wrote:

I didn't take the static functions and constants in the base class into
account while renaming. Of course, that looks confusing ;)

I will split the chunk into 3 classes:

  • CachingReaderChunk
  • CachingReaderChunkForOwner (i.e. the cache)
  • CachingReaderChunkForWorker (i.e. the worker thread)

On 07/28/2015 10:27 AM, Daniel Schürmann wrote:

Sorry, the CachingReaderChunk split still not works for me.

For example: CachingReaderChunkForWorker::indexForFrame is accessed from
cachingReader.

In the current state it looks more confusing than helpful to me.

A better solution may look like this, using two friend Keys:
http://stackoverflow.com/questions/1609472/friend-class-with-limited-access

Since the surrounding code is (thanks to all the refactoring and
comments) pretty clear now,
there is IMHO no need for such a split or any extra effort.
So what about renaming just CachingReaderChunkForWorker to
CachingReaderChunkBase change the comments accordingly and merge this PR?

By the way: The rest looks good to me. Thank you very much.


Reply to this email directly or view it on GitHub
#652 (comment).

@daschuer
Copy link
Member

OK, this will work for me as well.

@daschuer
Copy link
Member

LGTM Ready to merge?

@uklotzde
Copy link
Contributor Author

Ready. Further improvements can be done later.

@daschuer
Copy link
Member

Fine :-) Thank you for another nice PR.

daschuer added a commit that referenced this pull request Jul 28, 2015
CachingReader: Reduce log spam for corrupt files
@daschuer daschuer merged commit b3dd437 into mixxxdj:master Jul 28, 2015
@uklotzde
Copy link
Contributor Author

Thanks for the review, Daniel! Always a pleasure :)

On 07/28/2015 10:38 PM, Daniel Schürmann wrote:

Fine :-) Thank you for another nice PR.


Reply to this email directly or view it on GitHub
#652 (comment).

@uklotzde uklotzde deleted the ReduceLogSpamForCorruptFiles branch August 4, 2015 09:48
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.

2 participants