From 06d101316eaa187c9b85f596f2952477371502c5 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:02:41 +0000 Subject: [PATCH] fix(metadata): write tags into m4b files that carry mp3 audio FFmpeg picks its ipod muxer from the .m4b temp name, and that muxer rejects the MP3 an Audiobookshelf copy merge leaves inside an MP4. Probe the audio codec and use the mp4 muxer for anything but AAC, ALAC and AC-3, so other files keep their iTunes file type. The rename collision log now names the path that collided. Closes #1593 --- .../file-write/file-rename.service.test.ts | 3 + .../modules/file-write/file-rename.service.ts | 2 +- .../audio/audio-metadata-embedder.test.ts | 139 +++++++++++++++++- .../formats/audio/audio-metadata-embedder.ts | 57 +++++-- .../audio/audio-metadata-roundtrip.test.ts | 108 +++++++++++++- 5 files changed, 289 insertions(+), 20 deletions(-) diff --git a/server/src/modules/file-write/file-rename.service.test.ts b/server/src/modules/file-write/file-rename.service.test.ts index 0d04796ad..24f311193 100644 --- a/server/src/modules/file-write/file-rename.service.test.ts +++ b/server/src/modules/file-write/file-rename.service.test.ts @@ -1841,10 +1841,13 @@ describe('FileRenameService', () => { if (path.toString() === '/library/Frank Herbert/cover.jpg') return Promise.resolve(undefined); return Promise.reject(Object.assign(new Error('missing'), { code: 'ENOENT' })); }); + const warn = vi.spyOn(service['logger'], 'warn').mockImplementation(() => undefined); const result = await service.performRename(5, 12); expect(result.status).toBe('skipped'); expect(result.reason).toBe('target path already exists on disk'); + // The book's own file is free; the log must name the companion that collided. + expect(warn).toHaveBeenCalledWith(expect.stringContaining('newPath="/library/Frank Herbert/cover.jpg"')); }); describe('cancelPendingRename', () => { diff --git a/server/src/modules/file-write/file-rename.service.ts b/server/src/modules/file-write/file-rename.service.ts index 1686f049c..7d9588467 100644 --- a/server/src/modules/file-write/file-rename.service.ts +++ b/server/src/modules/file-write/file-rename.service.ts @@ -218,7 +218,7 @@ export class FileRenameService implements OnModuleDestroy { if (existingTargetPath) { const reason = 'target path already exists on disk'; this.logger.warn( - `[${FILE_RENAME_EVENT}] [end] bookId=${bookId} userId=${userId} durationMs=${Date.now() - startedAt} status=skipped reason="${sanitizeLogValue(reason)}" newPath="${sanitizeLogValue(newAbsolutePath)}" - rename skipped: target already exists on disk`, + `[${FILE_RENAME_EVENT}] [end] bookId=${bookId} userId=${userId} durationMs=${Date.now() - startedAt} status=skipped reason="${sanitizeLogValue(reason)}" newPath="${sanitizeLogValue(existingTargetPath)}" - rename skipped: target already exists on disk`, ); await this.notifyFailure(userId, bookId, `File rename skipped: ${reason}.`, suppressNotification); return { status: 'skipped', reason, oldPath: currentAbsolutePath, newPath: newAbsolutePath, durationMs: Date.now() - startedAt }; diff --git a/server/src/modules/file-write/formats/audio/audio-metadata-embedder.test.ts b/server/src/modules/file-write/formats/audio/audio-metadata-embedder.test.ts index a2ad64c6f..1448f46d4 100644 --- a/server/src/modules/file-write/formats/audio/audio-metadata-embedder.test.ts +++ b/server/src/modules/file-write/formats/audio/audio-metadata-embedder.test.ts @@ -174,9 +174,14 @@ describe('AudioMetadataEmbedder', () => { expect(probeCall[2]).toEqual({ timeout: 30_000 }); const ffmpegArgs = execFileMock.mock.calls.find(([bin]) => bin !== '/opt/bin/ffprobe')![1] as string[]; expect(ffmpegArgs).not.toContain('use_metadata_tags'); + expect(ffmpegArgs).not.toContain('-f'); }); - function mockProbedVideoStreams(stdout: string): void { + function mockProbedStreams(streams: Array<{ codec_type: string; codec_name?: string }>): void { + mockProbeOutput(JSON.stringify({ streams })); + } + + function mockProbeOutput(stdout: string): void { execFileMock.mockImplementation( (bin: string, _args: string[], _options: unknown, callback: (error: Error | null, result: { stdout: string; stderr: string }) => void) => { callback(null, { stdout: bin === 'ffprobe' ? stdout : '', stderr: '' }); @@ -184,6 +189,10 @@ describe('AudioMetadataEmbedder', () => { ); } + function ffmpegArgs(): string[] { + return execFileMock.mock.calls.find(([bin]) => bin === 'ffmpeg')![1] as string[]; + } + function mapArgs(args: string[]): string[] { return args.flatMap((arg, index) => (arg === '-map' ? [args[index + 1]!] : [])); } @@ -266,7 +275,10 @@ describe('AudioMetadataEmbedder', () => { }); it('adds freeform atoms when keeping existing art in an MP4', async () => { - mockProbedVideoStreams('0\n'); + mockProbedStreams([ + { codec_type: 'audio', codec_name: 'aac' }, + { codec_type: 'video', codec_name: 'mjpeg' }, + ]); const embedder = new AudioMetadataEmbedder(); await embedder.embedMetadata('/books/audio/book.m4a', 'm4a', { coverBytes: null, metadata: [{ key: 'publisher', value: 'Ace' }] }); @@ -277,15 +289,134 @@ describe('AudioMetadataEmbedder', () => { }); it('leaves extended tags to FFmpeg when it writes QuickTime metadata keys', async () => { - mockProbedVideoStreams(''); + mockProbedStreams([{ codec_type: 'audio', codec_name: 'aac' }]); const embedder = new AudioMetadataEmbedder(); await embedder.embedMetadata('/books/audio/book.m4b', 'm4b', { coverBytes: null, metadata: [{ key: 'subtitle', value: 'Book One' }] }); - expect(execFileMock.mock.calls.find(([bin]) => bin === 'ffmpeg')![1]).toContain('use_metadata_tags'); + expect(ffmpegArgs()).toContain('use_metadata_tags'); expect(mocks.writeMp4FreeformTags).not.toHaveBeenCalled(); }); + describe('MP4 output muxer', () => { + it.each(['mp3', 'mp2', 'opus', 'flac', 'eac3'])('writes %s audio with the mp4 muxer, which the ipod muxer rejects', (codec) => { + expect(testing.outputMuxer('m4b', [{ codecType: 'audio', codecName: codec }])).toBe('mp4'); + expect(testing.outputMuxer('m4a', [{ codecType: 'audio', codecName: codec }])).toBe('mp4'); + }); + + it.each(['aac', 'alac', 'ac3'])('leaves %s audio to the ipod muxer FFmpeg picks for the file name', (codec) => { + expect( + testing.outputMuxer('m4b', [ + { codecType: 'audio', codecName: codec }, + { codecType: 'video', codecName: 'mjpeg' }, + { codecType: 'data', codecName: 'bin_data' }, + ]), + ).toBeUndefined(); + }); + + it('uses the mp4 muxer when any audio stream is one the ipod muxer rejects', () => { + expect( + testing.outputMuxer('m4b', [ + { codecType: 'audio', codecName: 'aac' }, + { codecType: 'audio', codecName: 'mp3' }, + ]), + ).toBe('mp4'); + }); + + it('uses the mp4 muxer for audio FFmpeg could not name', () => { + expect(testing.outputMuxer('m4b', [{ codecType: 'audio', codecName: null }])).toBe('mp4'); + }); + + it('leaves the muxer alone when the streams are unknown or the format is not MP4', () => { + expect(testing.outputMuxer('m4b', null)).toBeUndefined(); + expect(testing.outputMuxer('mp3', [{ codecType: 'audio', codecName: 'mp3' }])).toBeUndefined(); + expect(testing.outputMuxer('flac', [{ codecType: 'audio', codecName: 'flac' }])).toBeUndefined(); + }); + + it('names the muxer immediately before the output path', () => { + const args = testing.buildFfmpegArgs('/a.m4b', '/c.jpg', '/t.m4b', 'm4b', [{ key: 'album', value: 'Book' }], { muxer: 'mp4' }); + + expect(args.slice(-3)).toEqual(['-f', 'mp4', '/t.m4b']); + expect(testing.buildFfmpegArgs('/a.m4b', '/c.jpg', '/t.m4b', 'm4b', [])).not.toContain('-f'); + }); + + it('probes an MP4 being given a new cover and writes its MP3 audio with the mp4 muxer', async () => { + mockProbedStreams([ + { codec_type: 'audio', codec_name: 'mp3' }, + { codec_type: 'data', codec_name: 'bin_data' }, + { codec_type: 'video', codec_name: 'mjpeg' }, + ]); + const embedder = new AudioMetadataEmbedder(); + + await embedder.embedMetadata('/books/audio/book.m4b', 'm4b', { + coverBytes: Buffer.from('raw-cover'), + metadata: [{ key: 'subtitle', value: 'Book One' }], + }); + + expect(execFileMock).toHaveBeenCalledWith( + 'ffprobe', + ['-v', 'error', '-show_entries', 'stream=codec_type,codec_name', '-of', 'json', '/books/audio/book.m4b'], + { timeout: 30_000 }, + expect.any(Function), + ); + expect(ffmpegArgs().slice(-3)).toEqual(['-f', 'mp4', '/books/audio/.bookorbit-write-fixed-id.m4b']); + expect(ffmpegArgs()).not.toContain('use_metadata_tags'); + expect(mocks.writeMp4FreeformTags).toHaveBeenCalledWith( + '/books/audio/.bookorbit-write-fixed-id.m4b', + [{ name: 'SUBTITLE', value: 'Book One' }], + { + carryOverFrom: '/books/audio/book.m4b', + }, + ); + expect(replaceFileAtomicallyMock).toHaveBeenCalledWith('/books/audio/.bookorbit-write-fixed-id.m4b', '/books/audio/book.m4b'); + }); + + it('writes an artless MP3 M4B with the mp4 muxer and FFmpeg metadata keys together', async () => { + mockProbedStreams([{ codec_type: 'audio', codec_name: 'mp3' }]); + const embedder = new AudioMetadataEmbedder(); + + await embedder.embedMetadata('/books/audio/book.m4b', 'm4b', { coverBytes: null, metadata: [{ key: 'subtitle', value: 'Book One' }] }); + + expect(ffmpegArgs()).toContain('use_metadata_tags'); + expect(ffmpegArgs().slice(-3)).toEqual(['-f', 'mp4', '/books/audio/.bookorbit-write-fixed-id.m4b']); + expect(mocks.writeMp4FreeformTags).not.toHaveBeenCalled(); + }); + + it('keeps the ipod muxer for an AAC M4B', async () => { + mockProbedStreams([ + { codec_type: 'audio', codec_name: 'aac' }, + { codec_type: 'video', codec_name: 'mjpeg' }, + ]); + const embedder = new AudioMetadataEmbedder(); + + await embedder.embedMetadata('/books/audio/book.m4b', 'm4b', { coverBytes: Buffer.from('raw-cover'), metadata: [] }); + + expect(ffmpegArgs()).not.toContain('-f'); + }); + + it.each([ + ['not JSON', 'Invalid data found when processing input'], + ['without a stream list', '{}'], + ])('treats probe output %s as unknown streams', async (_label, stdout) => { + mockProbeOutput(stdout); + const embedder = new AudioMetadataEmbedder(); + + await embedder.embedMetadata('/books/audio/book.m4b', 'm4b', { coverBytes: null, metadata: [{ key: 'subtitle', value: 'Book One' }] }); + + expect(ffmpegArgs()).not.toContain('-f'); + expect(ffmpegArgs()).not.toContain('use_metadata_tags'); + expect(mocks.writeMp4FreeformTags).toHaveBeenCalled(); + }); + + it.each(['mp3', 'flac'])('never probes a %s file', async (format) => { + const embedder = new AudioMetadataEmbedder(); + + await embedder.embedMetadata(`/books/audio/book.${format}`, format, { coverBytes: null, metadata: [] }); + + expect(execFileMock.mock.calls.map(([bin]) => bin)).toEqual(['ffmpeg']); + }); + }); + it.each(['mp3', 'flac'])('never edits MP4 atoms in %s files', async (format) => { const embedder = new AudioMetadataEmbedder(); diff --git a/server/src/modules/file-write/formats/audio/audio-metadata-embedder.ts b/server/src/modules/file-write/formats/audio/audio-metadata-embedder.ts index 7ed50b1a6..c735fae57 100644 --- a/server/src/modules/file-write/formats/audio/audio-metadata-embedder.ts +++ b/server/src/modules/file-write/formats/audio/audio-metadata-embedder.ts @@ -14,6 +14,13 @@ const execFile = promisify(execFileCallback); const FFMPEG_OUTPUT_MAX_BUFFER_BYTES = 10 * 1024 * 1024; const FFMPEG_TIMEOUT_MS = 60_000; const FFPROBE_TIMEOUT_MS = 30_000; +/** The only audio FFmpeg's ipod muxer accepts; an MP4 can also carry MP3, MP2, Opus, FLAC and E-AC-3. */ +const IPOD_MUXER_AUDIO_CODECS = new Set(['aac', 'alac', 'ac3']); + +interface ProbedStream { + codecType: string | null; + codecName: string | null; +} export interface AudioMetadataArg { key: string; @@ -44,12 +51,14 @@ export class AudioMetadataEmbedder { if (coverPath && input.coverBytes) { await writeFile(coverPath, await this.normalizedCover(input.coverBytes)); } - const useMp4MetadataTags = await shouldUseMp4MetadataTags(filePath, format, coverPath); - await execFile(resolveFfmpegPath(), buildFfmpegArgs(filePath, coverPath, tempPath, format, input.metadata, { useMp4MetadataTags }), { + const streams = isMp4Format(format) ? await probeStreams(filePath) : null; + const useMp4MetadataTags = shouldUseMp4MetadataTags(format, coverPath, streams); + const muxer = outputMuxer(format, streams); + await execFile(resolveFfmpegPath(), buildFfmpegArgs(filePath, coverPath, tempPath, format, input.metadata, { useMp4MetadataTags, muxer }), { maxBuffer: FFMPEG_OUTPUT_MAX_BUFFER_BYTES, timeout: FFMPEG_TIMEOUT_MS, }); - if (usesMp4MetadataTags(format) && !useMp4MetadataTags) { + if (isMp4Format(format) && !useMp4MetadataTags) { await writeMp4FreeformTags(tempPath, mp4FreeformTags(input.metadata), { carryOverFrom: filePath }); } await replaceFileAtomically(tempPath, filePath); @@ -80,7 +89,7 @@ function buildFfmpegArgs( tempPath: string, format: string, metadata: AudioMetadataArg[], - options?: { useMp4MetadataTags?: boolean }, + options?: { useMp4MetadataTags?: boolean; muxer?: string }, ): string[] { const args = ['-v', 'error', '-y', '-i', filePath]; @@ -102,7 +111,7 @@ function buildFfmpegArgs( args.push(entry.specifier ? `-metadata:${entry.specifier}` : '-metadata', `${entry.key}=${entry.value}`); } - const useMp4MetadataTags = options?.useMp4MetadataTags ?? (usesMp4MetadataTags(format) && !coverPath); + const useMp4MetadataTags = options?.useMp4MetadataTags ?? (isMp4Format(format) && !coverPath); if (useMp4MetadataTags) { // ffmpeg drops MP4 covr artwork when use_metadata_tags is combined with an attached picture stream. args.push('-movflags', 'use_metadata_tags'); @@ -112,6 +121,10 @@ function buildFfmpegArgs( args.push('-disposition:v:0', 'attached_pic', '-metadata:s:v:0', 'title=Album cover', '-metadata:s:v:0', 'comment=Cover (front)'); } + if (options?.muxer) { + args.push('-f', options.muxer); + } + args.push(tempPath); return args; @@ -124,26 +137,43 @@ function mp4FreeformTags(metadata: AudioMetadataArg[]): Mp4FreeformTag[] { .map((entry) => ({ name: entry.key.toUpperCase(), value: entry.value })); } -function usesMp4MetadataTags(format: string): boolean { +function isMp4Format(format: string): boolean { const normalized = format.toLowerCase(); return normalized === FORMAT_M4B || normalized === FORMAT_M4A; } -async function shouldUseMp4MetadataTags(filePath: string, format: string, coverPath: string | null): Promise { - if (!usesMp4MetadataTags(format) || coverPath) return false; - return !(await hasVideoStream(filePath)); +/** An unprobed file is treated as having art, which FFmpeg's iTunes mode would drop. */ +function shouldUseMp4MetadataTags(format: string, coverPath: string | null, streams: ProbedStream[] | null): boolean { + if (!isMp4Format(format) || coverPath || !streams) return false; + return !streams.some((stream) => stream.codecType === 'video'); } -async function hasVideoStream(filePath: string): Promise { +/** + * FFmpeg picks the ipod muxer for an .m4b or .m4a name, and it rejects audio such as the MP3 an + * Audiobookshelf merge stream-copies into an M4B. The mp4 muxer takes it; every other file keeps + * the ipod muxer and its iTunes file type. + */ +function outputMuxer(format: string, streams: ProbedStream[] | null): string | undefined { + if (!isMp4Format(format) || !streams) return undefined; + const ipodRejectsAudio = streams.some((stream) => stream.codecType === 'audio' && !IPOD_MUXER_AUDIO_CODECS.has(stream.codecName ?? '')); + return ipodRejectsAudio ? 'mp4' : undefined; +} + +async function probeStreams(filePath: string): Promise { try { const { stdout } = await execFile( resolveFfprobePath(), - ['-v', 'error', '-select_streams', 'v', '-show_entries', 'stream=index', '-of', 'csv=p=0', filePath], + ['-v', 'error', '-show_entries', 'stream=codec_type,codec_name', '-of', 'json', filePath], { timeout: FFPROBE_TIMEOUT_MS }, ); - return String(stdout).trim().length > 0; + const parsed = JSON.parse(String(stdout)) as { streams?: Array<{ codec_type?: unknown; codec_name?: unknown }> }; + if (!Array.isArray(parsed.streams)) return null; + return parsed.streams.map((stream) => ({ + codecType: typeof stream.codec_type === 'string' ? stream.codec_type : null, + codecName: typeof stream.codec_name === 'string' ? stream.codec_name : null, + })); } catch { - return true; + return null; } } @@ -169,4 +199,5 @@ async function removeFileIfPresent(path: string): Promise { export const testing = { buildFfmpegArgs, + outputMuxer, }; diff --git a/server/src/modules/file-write/formats/audio/audio-metadata-roundtrip.test.ts b/server/src/modules/file-write/formats/audio/audio-metadata-roundtrip.test.ts index df1545858..1ce8a7d92 100644 --- a/server/src/modules/file-write/formats/audio/audio-metadata-roundtrip.test.ts +++ b/server/src/modules/file-write/formats/audio/audio-metadata-roundtrip.test.ts @@ -344,7 +344,111 @@ describe('audio metadata round-trip', () => { expect(await extractArt(filePath)).toEqual(artBefore); }); - async function generateChapteredAudioFixture(filePath: string, coverPath: string): Promise { + // Audiobookshelf's merge stream-copies MP3 tracks into an MP4, leaving MP3 audio the ipod muxer rejects. + describe.each([ + ['m4b', M4bAudioFormatWriter], + ['m4a', M4aAudioFormatWriter], + ] as const)('a %s whose audio is MP3', (format, Writer) => { + it('takes new tags and a replacement cover, keeping the MP3 audio and the chapters', async () => { + const filePath = join(tempDir, `merged.${format}`); + const coverPath = join(tempDir, 'cover.jpg'); + await generateCoverFixture(coverPath); + await generateChapteredAudioFixture(filePath, coverPath, 'libmp3lame'); + + const writer = new Writer(new AudioMetadataEmbedder()); + await expect( + writer.write( + filePath, + { + title: 'Merged Title', + subtitle: 'Merged Subtitle', + seriesName: 'Merged Series', + seriesIndex: '3', + coverBytes: await readFile(coverPath), + }, + { dryRun: false, fieldMask: createBookWriteFieldMask() }, + ), + ).resolves.toEqual(expect.objectContaining({ status: 'success' })); + + const streams = await probeStreams(filePath); + expect(streams.find((stream) => stream.codec_type === 'audio')?.codec_name).toBe('mp3'); + expect(streams.find((stream) => stream.codec_type === 'video')?.disposition?.attached_pic).toBe(1); + expect(await probeChapterTitles(filePath)).toEqual(['One', 'Two']); + + vi.resetModules(); + const { extractAudioMetadata } = await import('../../../metadata/extractors/audio.extractor'); + const extracted = await extractAudioMetadata(filePath); + expect(extracted).toEqual( + expect.objectContaining({ title: 'Merged Title', subtitle: 'Merged Subtitle', seriesName: 'Merged Series', seriesIndex: '3' }), + ); + expect(extracted.coverBytes?.length).toBeGreaterThan(0); + }); + + it('takes new tags without a cover, keeping its chapters and existing art', async () => { + const filePath = join(tempDir, `merged.${format}`); + const coverPath = join(tempDir, 'cover.jpg'); + await generateCoverFixture(coverPath); + await generateChapteredAudioFixture(filePath, coverPath, 'libmp3lame'); + const artBefore = await extractArt(filePath); + + const writer = new Writer(new AudioMetadataEmbedder()); + await expect( + writer.write( + filePath, + { title: 'Tag Only', subtitle: 'Tag Only Subtitle', coverBytes: null }, + { dryRun: false, fieldMask: createBookWriteFieldMask() }, + ), + ).resolves.toEqual(expect.objectContaining({ status: 'success' })); + + expect(await probeChapterTitles(filePath)).toEqual(['One', 'Two']); + expect(await extractArt(filePath)).toEqual(artBefore); + expect(await probeFormatTags(filePath)).toEqual(expect.objectContaining({ title: 'Tag Only', subtitle: 'Tag Only Subtitle' })); + }); + + it('takes new tags when it carries no art at all', async () => { + const filePath = join(tempDir, `bare.${format}`); + await execFile(ffmpegPath, [ + '-v', + 'error', + '-y', + '-f', + 'lavfi', + '-i', + 'sine=frequency=1000:duration=0.2', + '-c:a', + 'libmp3lame', + '-f', + 'mp4', + filePath, + ]); + + const writer = new Writer(new AudioMetadataEmbedder()); + await expect( + writer.write(filePath, { title: 'Bare Title', subtitle: 'Bare Subtitle' }, { dryRun: false, fieldMask: createBookWriteFieldMask() }), + ).resolves.toEqual(expect.objectContaining({ status: 'success' })); + + expect(await probeFormatTags(filePath)).toEqual(expect.objectContaining({ title: 'Bare Title', subtitle: 'Bare Subtitle' })); + expect((await probeStreams(filePath)).find((stream) => stream.codec_type === 'audio')?.codec_name).toBe('mp3'); + }); + }); + + it('keeps the iTunes file type when rewriting an AAC m4b', async () => { + const filePath = join(tempDir, 'itunes.m4b'); + const coverPath = join(tempDir, 'cover.jpg'); + await generateCoverFixture(coverPath); + await generateChapteredAudioFixture(filePath, coverPath, 'aac'); + + const writer = new M4bAudioFormatWriter(new AudioMetadataEmbedder()); + await writer.write( + filePath, + { title: 'Branded', coverBytes: await readFile(coverPath) }, + { dryRun: false, fieldMask: createBookWriteFieldMask() }, + ); + + expect((await probeFormatTags(filePath)).major_brand?.trim()).toBe('M4A'); + }); + + async function generateChapteredAudioFixture(filePath: string, coverPath: string, audioCodec = 'aac'): Promise { const chaptersPath = join(tempDir, 'chapters.txt'); await writeFile( chaptersPath, @@ -371,7 +475,7 @@ describe('audio metadata round-trip', () => { '-map_chapters', '2', '-c:a', - 'aac', + audioCodec, '-b:a', '64k', '-c:v',