Skip to content

Conversation

@joshtrichards
Copy link
Contributor

Thanks (once again) for the useful library we use over at the Nextcloud project!

While looking into nextcloud/server#44573, I noted we might as well throw an informative exception if the codecVersion, layerVersion, or channel headers are unrecognized.

We're going to error out anyway when we try to use empty values for the array key. Instead of generating generic PHP errors when we go to set the bitRate and sampleRate or vbr_offset we can leave the user a bit less confused about the underlying cause.

I don't even know what MP3s might cause this, but seems reasonable to handle this situation cleanly if possible. :)

@jeremitu
Copy link

This is not sufficient, we need to consider CODEC_UNDEFINED, which is not covered by the tables:

        if ($this->codecVersion == self::CODEC_UNDEFINED) {
            throw new \Exception('Codec undefined');
        }

@kesselb
Copy link
Contributor

kesselb commented Jun 13, 2024

@joshtrichards mind to update your pr with jeremitu's suggestion?

I will patch the copy in nextcloud/3rdparty then.

@joshtrichards
Copy link
Contributor Author

Done.

Rather than add a check for it, I dropped CODEC_UNDEFINED outright in the parser. It's "documented" only as reserved from all the mp3 header docs I can find online, including the one referenced from this repo @ http://mpgedit.org/mpgedit/mpeg_format/mpeghdr.htm

It got added as part of the fix for #14, but that was for MPEG_25 and I think CODEC_UNDEFINED just got tossed in there for completeness at the time. We're not bothering to parse the the "reserved" ones elsewhere (e.g. for the layerVersion) so I don't see why it makes sense to do it for the codecVersion (particularly given all we can do is fail on it later anyhow).

kesselb added a commit to nextcloud/3rdparty that referenced this pull request Jul 3, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb added a commit to nextcloud/server that referenced this pull request Jul 3, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb added a commit to nextcloud/server that referenced this pull request Jul 4, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
backportbot bot pushed a commit to nextcloud/3rdparty that referenced this pull request Jul 4, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

[skip ci]
kesselb added a commit to nextcloud/3rdparty that referenced this pull request Jul 4, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb added a commit to nextcloud/3rdparty that referenced this pull request Jul 4, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb added a commit to nextcloud/server that referenced this pull request Jul 4, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb added a commit to nextcloud/server that referenced this pull request Jul 6, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
backportbot bot pushed a commit to nextcloud/3rdparty that referenced this pull request Jul 6, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

[skip ci]
kesselb added a commit to nextcloud/server that referenced this pull request Jul 6, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
joshtrichards pushed a commit to nextcloud/server that referenced this pull request Jul 7, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb added a commit to nextcloud/server that referenced this pull request Jul 8, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb added a commit to nextcloud/server that referenced this pull request Jul 8, 2024
Patch: wapmorgan/Mp3Info#36

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@wapmorgan wapmorgan merged commit e532c2e into wapmorgan:master Apr 1, 2025
@wapmorgan
Copy link
Owner

@joshtrichards c c released 0.1.1 with this fix

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.

4 participants