-
Notifications
You must be signed in to change notification settings - Fork 77
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
Added genre parsing for mp4 (atom) file metadata #83
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the contribution! Just a few quick changes.
@@ -201,7 +640,7 @@ func (m metadataMP4) readAtomData(r io.ReadSeeker, name string, size uint32, pro | |||
if len(b) < 1 { | |||
return fmt.Errorf("invalid encoding: expected at least %d bytes, for integer tag data, got %d", 1, len(b)) | |||
} | |||
data = getInt(b[:1]) | |||
data = getInt(b[len(b)-1:]) |
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.
Is there some padding in front that we need to jump over? Might be best to explicitly say that (for future reference!)
if len(b) < 4 { | ||
return fmt.Errorf("invalid encoding: expected at least %d bytes, for class, got %d", 4, len(b)) | ||
} | ||
|
||
if name == "gnre" { | ||
m.data[name] = getInt(b[len(b)-1:]) |
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.
Jumping over something here?
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.
The genre identifier in the "gnre" tag is stored at the end of the byte array, subsequent processing is redundant, we only need id
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.
Ah ok. But then b[len(b)-1:]
is going to be 1 byte
, so the number will only ever be between 0 and 255 (which will not hit any of the genres added). Is this is in the spec somewhere?
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.
Yes, the identifier is stored in one byte.
Sorry, couldn't find an explanation / specification.
I can provide an example.
Co-authored-by: David Howden <dhowden@gmail.com>
if len(b) < 4 { | ||
return fmt.Errorf("invalid encoding: expected at least %d bytes, for class, got %d", 4, len(b)) | ||
} | ||
|
||
if name == "gnre" { | ||
m.data[name] = getInt(b[len(b)-1:]) |
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.
Ah ok. But then b[len(b)-1:]
is going to be 1 byte
, so the number will only ever be between 0 and 255 (which will not hit any of the genres added). Is this is in the spec somewhere?
No description provided.