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

Add filepointer option to openfile/analyze function #172

Closed
Rello opened this issue Dec 4, 2018 · 7 comments
Closed

Add filepointer option to openfile/analyze function #172

Rello opened this issue Dec 4, 2018 · 7 comments

Comments

@Rello
Copy link
Contributor

Rello commented Dec 4, 2018

Hello James,

first a thank you for your super library.

I am using it in the Audio Player for Nextcloud/ownCloud. Same does @paulijar for the Music app.

We both use a little tweak to hand over a file pointer for better performance using streams.
As we have to apply the tweak after every of your nice upgrades, I would like to ask you to review this to evaluate if you could include this from stock. It might be useful for others also.

you can see the modifications here: Rello/audioplayer@89f6a26

Please let me know what you think.
Thank you,
Rello

@JamesHeinrich
Copy link
Owner

I would suggest that instead of breaking the definition of function openfile by inserting a new parameter between the two existing ones, add $fp=null as the third parameter, not the second. That way any existing calls to the function will function unchanged, and only any modified calls passing $fp as the third parameter will use the new code.

If you want to submit the change (modified as per above) as a pull request I can merge it. Please be sure to maintain my finicky formatting standards, with a space between parameters:
openfile($filename, null, $fp) not openfile($filename,null,$fp)
and explicit logic parentheses:
no: if ($fp != null && (get_resource_type($fp) == 'file' || get_resource_type($fp) == 'stream')) {
yes: if (($fp != null) && ((get_resource_type($fp) == 'file') || (get_resource_type($fp) == 'stream'))) {

@Rello
Copy link
Contributor Author

Rello commented Dec 4, 2018

hello,
thank you for your quick reply!

yes, as a third parameter is the prerequisite for a global change. Did not want to make my first post even bigger.

I will create a pull.
thanks and keep up with your nice work...

@Rello
Copy link
Contributor Author

Rello commented Dec 4, 2018

Hello James,
I forgot to bump the version number.
I guess it will be 1.9.16-201812042150 ?
will you change it? then I will include the new version right away...

@JamesHeinrich
Copy link
Owner

Sure, that can be the version number. I made the version number change, wrapped into an omnibus code-formatting cleanup commit:
aab2924

@JamesHeinrich
Copy link
Owner

This change has broken some other formats that spawn a second instance of getID3 to examine additional content inside a container (e.g. mp3 audio inside RIFF.AVI, or WavPack audio inside RIFF.WAV). I haven't yet found the cause or solution, but I'll look at it tomorrow.

@JamesHeinrich JamesHeinrich reopened this Dec 5, 2018
JamesHeinrich added a commit that referenced this issue Dec 5, 2018
@JamesHeinrich
Copy link
Owner

Actually it wasn't so hard, there was an unnecessary addition of the filepointer to openfile under getid3_riff::ParseRIFFdata, the temp file opened there should always be local.
Fixed in f31b896

@Rello
Copy link
Contributor Author

Rello commented Dec 5, 2018

sorry for that. did not have this combination in my testfiles

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

No branches or pull requests

2 participants