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

x2 speed reading ID3v2 tags. #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

x2 speed reading ID3v2 tags. #26

wants to merge 1 commit into from

Conversation

Feverqwe
Copy link

Need add ID3v2.readSynchsafeInteger32At = readSynchsafeInteger32At;
in ID3v2

Need add ID3v2.readSynchsafeInteger32At = readSynchsafeInteger32At;
in ID3v2
@Feverqwe
Copy link
Author

@aadsm
Copy link
Owner

aadsm commented Nov 26, 2013

The FileReader should just be there to provide data and shouldn't deal with tags reading logic.
The ID3v{2,1}/ID4 already have a similar logic to what you wrote here, they call loadRanges() on the BinaryFile given to allow the data provider to only fetch the data that is strictly needed.

I think the best option is to create something similar to BufferedBinaryFile (this one only works for XHR). But the idea is the same, extend BinaryFile and implement loadRanges() in a way that only reads the needed data from the file.

https://github.com/aadsm/JavaScript-ID3-Reader/blob/master/src/bufferedbinaryajax.js#L231-L240

The current architecture is not ideal but it served its purpose at the time, I haven't had the time to rewrite it, so it is what it is :-)

@Feverqwe
Copy link
Author

I'am add BufferedBinaryFileReader!
This draft, but works perfectly!

Note, have big problem with getBlockAtOffset, I don't know how fix it, need callback.

https://github.com/Feverqwe/JavaScript-ID3-Reader/commit/768928004cfa0cfe059b2f01fe91426f93e9e86a

@aadsm
Copy link
Owner

aadsm commented Nov 26, 2013

It's definitely a step in the right direction but I was not thinking about creating a new file option.
Let me look at it more carefully by the end of the week as I don't have the time to do it right now.

@aadsm
Copy link
Owner

aadsm commented Nov 29, 2013

I've used your code to create a BufferedFileAPIReader, I created a branch with it, can you try it out? https://github.com/aadsm/JavaScript-ID3-Reader/tree/pr-26
So, basically, instead of using the FileAPIReader you use the BufferedFileAPIReader.
I'll merge both into one when this is read to go to master. There was a bug report on the Buffered code that manages the chunks that I still haven't looked into it, so I'm cautious with that code. I'm working on building a test suite on my local machine.

What do you mean with having a big problem with getBlockAtOffset?

There's still a lot of duplicated code, the problem of not using prototype inheritance...

@Feverqwe
Copy link
Author

When I tested this code, part of the images were broken, it's probably due to the fact that not all the relevant part of the file is loaded.
I tried to delete function getBlockRangeForByteRange, and load only the blocks, but then everything goes wrong. Need to pre-load a portion of the file that will be read. In the case of ID3v2 and ID3v1 to do it simply. This requires a new function in daraReader and many changes in ID3v2,ID3v1 and ID4.

I will try to implement this option, but it requires some time.

@aadsm
Copy link
Owner

aadsm commented Nov 30, 2013

I didn't have any problems in my tests, I was able to read images just fine.
Preload is the function of the loadRange(range, callback). The ID3v2 calls this function when it needs to read data.
There's probably a bug in the block ranges (or maybe ID3v2 is requesting/reading the wrong range of bytes), I can try to implement a simpler version of it.

@Feverqwe
Copy link
Author

You won't have problems because blockSize in most cases covers the required part. Need to change some of the range for loadRange then blockSize is not required.

If you would be interested, I slightly changed the way of getting the picture, this works much better than the one that was before, but changes the type of the output ( from getBytesAt to getStringAt ).
https://github.com/Feverqwe/JavaScript-ID3-Reader/blob/master/src/id3v2frames.js#L96-L115

And I write script for windows
https://github.com/Feverqwe/JavaScript-ID3-Reader/blob/master/make-minimize.bat

@Feverqwe
Copy link
Author

I'am update bufferedbinaryfilereader, remove blockSize, add Uint8Array, and now it very small and fast reader =) https://github.com/Feverqwe/JavaScript-ID3-Reader/commit/d76672bd57e50ac612242664e6f74a0320d61617

ID3v2 ID3v1 and should work well, as they have the specified size. ID4 like also works not bad. I made a small change in ID3v2.

upd. now work ID4, many fix's with ranges. https://github.com/Feverqwe/JavaScript-ID3-Reader/commit/941ebc1d0de9781ba123dc0ce5fb2cd2b8515a80 see last version file (I can change something.).

@aadsm
Copy link
Owner

aadsm commented Nov 30, 2013

Great work Anton, thank you so much! This is what I was going to do this morning but even more :-)
I can't believe that for 3 years I've had that bug about reading 32bits out of 2bytes!

I've added comments to your two commits, I don't think I understand the motivation behind the changes in ID4.

@Feverqwe
Copy link
Author

Just.. uncomment console.log in bufferedbinaryfilereader.js, and it show all unloaded blocks, it don't all fix's, but it work. I sometimes difficult to understand code and specifications, but it seems now it works not bad.

@aadsm
Copy link
Owner

aadsm commented Nov 30, 2013

Thanks for all your comments and fixes (it seems that I can't do basic math :D)
Going to merge all your stuff in but you made your fixes in bufferedbinaryfileread.js not in bufferedfilereader.js, (I prefer a different data reader).
I think I'll just apply those changes to the bufferedfilereader.js and put you as the author, if that's ok with you.

Greetings from California to Ufa (which is one of the Russian cities marked in my world map shower curtain!)

@Feverqwe
Copy link
Author

yes bufferedfilereader.js sounds better =) Thanks for a great library.

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