-
Notifications
You must be signed in to change notification settings - Fork 275
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
headers for remote streams (including volatile) #367
Merged
Merged
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
eeb3f26
rebase
philippe44 6d6201c
typo + stop AAC parser if reached end of stsz table
philippe44 363cf0f
comments & tweaks
philippe44 fe3ba9f
move parsing to Audio::Scan
philippe44 44f9376
Linux requires seeking (and maybe a file handle copy, not sure why)
philippe44 1ff06f2
complete parsing move to Format::XXX
philippe44 5d79e33
integrate stash creation to audio_process
philippe44 e545b8b
proper demux process intialization
philippe44 6589f8f
removing unused bits
philippe44 72edf2d
add WIMP support and a bit of cleanup
philippe44 ffa016a
load classes in header parsing
philippe44 54b8323
really load class (this time) and add live header parsing
philippe44 54b69eb
tweak on-the-fly header + WIMP fix for direct streaming
philippe44 4eca380
forgot to create local mapping for _content_type
philippe44 08d0a45
addressing style comments
philippe44 401bb80
consolidate HTTP/HTTPS sysread + add canDirectStreamSong in HTTPS
philippe44 e26a444
handle recurse issue in HTTPS for ICY + avoid reference in InitialBlock
philippe44 b44a8c1
solve sysread issue + unlike temp files
philippe44 a74908e
Thank you Windows...
philippe44 311256a
comment + remove use os smartmatch
philippe44 2c6582b
avoid init invocation + prepare 'reliable'
philippe44 e26cb69
debug sentence fix (no auto invoke!)
philippe44 3a225bd
comment clarification
philippe44 5ffa970
improve comments
philippe44 474ec2b
don't stash range unless self is blessed (direct streaming)
philippe44 3bac645
streamUrl must be updated when scanUrl redirect/changes url
philippe44 7ca3490
Merge branch 'public/8.0' into remote-headers
philippe44 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still believe that this file handle risks to never be released. The documentation for
File::Temp
says:But when would this be closed? We don't close it actively. Would it be closed when the track object is discarded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use
Slim::Player::Song::DESTROY
to close the handle. Something like this:This effectively got rid of the temporary files.
close $track->initial_block_fh
did not work, for whatever reason.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think RemoteTrack::delete could be used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also tried to add a DESTROY method to RemoteTrack but it does not seem to be called. I have to check the Song as well but I think I remember in a previous modification, when I was trying to understand how all this works, I tested this liveCount and I never saw it decreased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RemoteTrack::delete()
is more than that. I actually was wondering whether that information/file handle should belong to the song object rather than the track, as it's only relevant when the track is being played. The track object can be around for much longer, eg. when it's queued up, or after it had been played, it would still be part of the current playlist. But the file handle is not required at that point.RemoteTrack
'sDESTROY
probably isn't called for a long time, as these objects are kept around for a while for aforementioned reasons.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think this fh is really an attribute of the track that must 'survive' between $song instances as the url might not be rescanned. So if I delete it, I won't be able to re-create it. And when I populate the track information, I might not have a song object. For example, scanUrl does not always have one when called, as it can scan multiple tracks, keep them ready and then create song when getNextTrack/Song is called. It's like for File.pm, the audio_offset and audio_size (e.g.) are attributes that are created independently of a song object. But I agree that we don't want to keep them around forever. I'm trying to find under which circumstances the $track object is destroyed (other than the call to delete which obviously happen when the track changes). At least, when LMS stops, things are cleaned, I verified :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to see whether
RemoteTrack
'sDESTROY
would be called or not you could reduce the size of%Cache
in that file. It's initially set to 500 items, but ininit()
's$maxPlaylistLengthCB
you could set it to any small size to make sure it soon would need to run some cleanup.And yes, I've seen LMS clean up behind, too. But some people listen to hundreds of tracks before they restart LMS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will try that. Maybe I could as well have a global that tracks the total size of these headers and kicks in some DESTROY when a max is reached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I did not know it is possible to register a callback on a prefs change ... there are so many well thought features in LMS. I've discovered a couple of inconsistencies in codec management between pcm/wav/aif and mp4/aac but these are very minor compared to extremely well done global architecture, IMHO