Comments (7)
Hmm. That's an interesting issue. I'll take a look shortly to see some more of the details, but I thought I'd give an explanation why that check is there (and I'll do my best off the top of my head). Basically, the granular position ends up being an offset into the file. While newer versions of node support bigint for file offsets, the version I'm currently targeting does not. As such, any file offsets are stored in node-taglib-sharp as regular numbers, and generally this check exists to make sure we don't try to work with files that are bigger than that version of node can support. I'm not entirely sure the behavior of the fs
functions if you provide it an unsafe integer.
What's interesting is that 0n
is bigger than MAX_SAFE_INTEGER
. Let me look into a better way to make this check, and I'll see about getting a patch fix out.
Lastly, thanks for your interest in this project, thoroughly looking into the code, and giving me a sample file! Makes my life easier and makes me feel good someone's actually trying this project!
from node-taglib-sharp.
Thank you very much for explaining the reason behind this check. It's indeed interesting why 0n is considered larger than MAX_SAFE_INTEGER. I've done a thorough search on the internet and couldn't yet find an explanation.
You're very welcome. I love your library. I've been using TagLib# for years for an audio player that runs solely on Windows. I've since created a multiplatform version too, using Electron and uses node-taglib-sharp.
from node-taglib-sharp.
Initial findings - the first page that's parsed has a granule position of 0n
which proceeds successfully. The second one appears to have a granule position of FF FF FF FF FF FF FF FF
which is definitely bigger than MAX_SAFE_INTEGER
if processed as a ulong. After digging into the spec, it appears that this is a special value
A special value of -1 (in two's complement) indicates that no packets finish on this page.
So it looks like this page might be able to be thrown out or something. I'll try to figure out the implications of this later today.
from node-taglib-sharp.
Ok, I've taken a solid look the implications, and it looks like this is just a special granule position value. As far as I can tell (and this digs into the code the taglib# devs prefaced with "if you don't understand this, you are not alone, it is confusing as hell"), the granule position passes through without any modification, so simply detecting the special value and handling it seems fine.
I was mostly wrong in saying that the granule position corresponds to a file offset. The granule position has a different meaning to each ogg codec, but for our purposes, it can be used to calculate the duration of the file. Although some fancy logic applies, it's basically last granule position minus first granule position. And what's more, the 0xFFFFFFFFFFFFFFFF value would only appear in the middle of a file and wouldn't impact the calculation of duration. As such, I don't need to limit this field to a number
. However, that's a bigger change, so I'll table it for the moment in favor of a small fix to unblock you.
As a sidenote, the original taglib# pagination code doesn't consider the case of all page segments being 0xFF. If the paginator generates a page like that, the granule position should be changed to 0xFFFFFFFFFFFFFFFF. I'm guessing it is a super unlikely situation, but I wonder how it would impact the file.
Lastly, I've got a hotfix branch ready hotfix/ogg-bigint-comparison, if you'd like to test it. Otherwise, I'll merge it in and have v5.0.1 out shortly.
from node-taglib-sharp.
Thank you so much for looking into this and for providing the fix. I must be honest and admit I don't fully understand granule positions and pagination (I'm somewhat relieved though that the TagLib# developers feel the same way). I do understand the modifications though that you've applied to handle this case. Very clean!
I tested your fix and it works perfectly. Thanks again!
from node-taglib-sharp.
Excellent, thanks for testing it! I've got just published the new version on npm, https://www.npmjs.com/package/node-taglib-sharp
So for now, I'm going to close this issue and open a new one to track removing the safe integer check all together.
I also noticed you're working on adding the MPEG support I haven't been able to get to - I'm absolutely flattered! Feel free to reach out via email if you have any questions. I'm really excited someone else is interested in contributing 😄
from node-taglib-sharp.
Thank you for publishing a new version! I'll integrate it in my project. It'll allow me to close a bug :)
I'm indeed attempting to add mpeg4 support. I planned to notify you as soon as I got a little further, as I'm honestly not sure yet where this will lead me. They have some challenging inheritance going on with the Boxes. But I really want to get this to work and won't hesitate to contact you if I get stuck. Thank you.
You really did a tremendous job on this project. The current implementation (the helper classes like ByteVector and File as well as the implementation of the other file formats) really helps for the conversion of the mpeg4 implementation from C# to TypeScript.
from node-taglib-sharp.
Related Issues (20)
- Version 5.2.3 breaks reading of mp3 picture.data for some files HOT 4
- bitrate will be 0 on some files HOT 2
- Error setting tags for m4a files HOT 26
- Remove Ogg granule position safe integer check and make granule position a number HOT 1
- Support for M4A HOT 7
- use in browser HOT 5
- Matroska tag docs didn't get generated HOT 1
- Question - error setting title HOT 2
- Question - Setting the text information frames in a id3v2TextInformationFrame HOT 2
- Id3v2FrameIdentifier doc and possible bug ? HOT 4
- Named export not found HOT 4
- Pictures cannot display in windows file browser. HOT 4
- ID3v2 tags at end of file not showing up in players
- Throwing error: `Argument null: ${name} was not provided` HOT 10
- 'Cannot set property ByteVector' error on load of module HOT 3
- id3v2 frameId RGAD causes failure to read tags HOT 4
- Add Legacy ReplayGain Frame Support
- Why Tag class has no accessor for 'artist' ? HOT 1
- poor performance when saving picture HOT 3
- wrong duration of `.mp3` and `.wav` HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from node-taglib-sharp.