From cde708005240a3b6b976cae8a281b83ff14cbeaf Mon Sep 17 00:00:00 2001 From: gpulch <42658765+gpulch@users.noreply.github.com> Date: Mon, 23 Feb 2026 10:49:05 +0100 Subject: [PATCH] fix: address code review feedback Inline fixes: - Remove TDRC frame from ID3 writer (ID3v2.3 uses TYER only, not TDRC) - Add try/finally cleanup in worker to prevent VFS leaks on errors - Fix Blob creation to use Uint8Array directly (avoid extra bytes) - Replace loadFFmpeg race guard with promise singleton pattern - Add -map_metadata -1 to strip source metadata (prevent duplicate ID3) Error handling improvements: - Create MP3EncodingError class with code property for reliable detection - Update api.js to use instanceof check instead of string matching - Pass AbortSignal to encodeToMp3 for proper cancellation support - Remove error re-wrapping in mp3-encoder.js (preserve original errors) Technical details: - Promise singleton ensures FFmpeg loads once even with concurrent calls - AbortSignal listener properly cleaned up on success/error/abort - Virtual FS cleanup in finally block prevents file leaks - MP3EncodingError.code = 'MP3_ENCODING_FAILED' for robust detection --- js/api.js | 8 +-- js/id3-writer.js | 1 - js/mp3-encoder.js | 41 ++++++++++++--- js/mp3-encoder.worker.js | 107 ++++++++++++++++++++++----------------- 4 files changed, 98 insertions(+), 59 deletions(-) diff --git a/js/api.js b/js/api.js index 27b2322..521e600 100644 --- a/js/api.js +++ b/js/api.js @@ -10,7 +10,7 @@ import { trackDateSettings } from './storage.js'; import { APICache } from './cache.js'; import { addMetadataToAudio } from './metadata.js'; import { DashDownloader } from './dash-downloader.js'; -import { encodeToMp3 } from './mp3-encoder.js'; +import { encodeToMp3, MP3EncodingError } from './mp3-encoder.js'; export const DASH_MANIFEST_UNAVAILABLE_CODE = 'DASH_MANIFEST_UNAVAILABLE'; const TIDAL_V2_TOKEN = 'txNoH4kkV41MfH25'; @@ -1137,7 +1137,7 @@ export class LosslessAPI { // Convert to MP3 320kbps if requested if (quality === 'MP3_320') { try { - blob = await encodeToMp3(blob, onProgress); + blob = await encodeToMp3(blob, onProgress, options.signal); } catch (encodingError) { if (onProgress) { onProgress({ @@ -1145,7 +1145,7 @@ export class LosslessAPI { message: `Encoding failed: ${encodingError.message}`, }); } - throw new Error(`MP3 encoding failed: ${encodingError.message}`); + throw encodingError; } } @@ -1176,7 +1176,7 @@ export class LosslessAPI { throw error; } console.error('Download failed:', error); - if (error.message && error.message.startsWith('MP3 encoding failed:')) { + if (error instanceof MP3EncodingError || error.code === 'MP3_ENCODING_FAILED') { throw error; } if (error.message === RATE_LIMIT_ERROR_MESSAGE) { diff --git a/js/id3-writer.js b/js/id3-writer.js index 343b8cf..ffaf394 100644 --- a/js/id3-writer.js +++ b/js/id3-writer.js @@ -29,7 +29,6 @@ async function writeID3v2Tag(mp3Blob, metadata, coverBlob = null) { const year = new Date(metadata.album.releaseDate).getFullYear(); if (!Number.isNaN(year) && Number.isFinite(year)) { frames.push(createTextFrame('TYER', year.toString())); - frames.push(createTextFrame('TDRC', year.toString())); } } diff --git a/js/mp3-encoder.js b/js/mp3-encoder.js index 0a1d3ff..d5549d6 100644 --- a/js/mp3-encoder.js +++ b/js/mp3-encoder.js @@ -1,18 +1,42 @@ -async function encodeToMp3Worker(audioBlob, onProgress = null) { +class MP3EncodingError extends Error { + constructor(message) { + super(message); + this.name = 'MP3EncodingError'; + this.code = 'MP3_ENCODING_FAILED'; + } +} + +async function encodeToMp3Worker(audioBlob, onProgress = null, signal = null) { const audioData = await audioBlob.arrayBuffer(); return new Promise((resolve, reject) => { const worker = new Worker(new URL('./mp3-encoder.worker.js', import.meta.url), { type: 'module' }); + // Handle abort signal + const abortHandler = () => { + worker.terminate(); + reject(new MP3EncodingError('MP3 encoding aborted')); + }; + + if (signal) { + if (signal.aborted) { + abortHandler(); + return; + } + signal.addEventListener('abort', abortHandler); + } + worker.onmessage = (e) => { const { type, blob, message, stage, progress } = e.data; if (type === 'complete') { + if (signal) signal.removeEventListener('abort', abortHandler); worker.terminate(); resolve(blob); } else if (type === 'error') { + if (signal) signal.removeEventListener('abort', abortHandler); worker.terminate(); - reject(new Error(message)); + reject(new MP3EncodingError(message)); } else if (type === 'progress' && onProgress) { onProgress({ stage, message, progress }); } else if (type === 'log') { @@ -21,8 +45,9 @@ async function encodeToMp3Worker(audioBlob, onProgress = null) { }; worker.onerror = (error) => { + if (signal) signal.removeEventListener('abort', abortHandler); worker.terminate(); - reject(new Error('Worker failed: ' + error.message)); + reject(new MP3EncodingError('Worker failed: ' + error.message)); }; // Transfer audio data to worker @@ -32,16 +57,18 @@ async function encodeToMp3Worker(audioBlob, onProgress = null) { }); } -export async function encodeToMp3(audioBlob, onProgress = null) { +export async function encodeToMp3(audioBlob, onProgress = null, signal = null) { try { // Use Web Worker for non-blocking FFmpeg encoding if (typeof Worker !== 'undefined') { - return await encodeToMp3Worker(audioBlob, onProgress); + return await encodeToMp3Worker(audioBlob, onProgress, signal); } - throw new Error('Web Workers are required for MP3 encoding'); + throw new MP3EncodingError('Web Workers are required for MP3 encoding'); } catch (error) { console.error('MP3 encoding failed:', error); - throw new Error('Failed to encode MP3: ' + error.message); + throw error; } } + +export { MP3EncodingError }; diff --git a/js/mp3-encoder.worker.js b/js/mp3-encoder.worker.js index 6c02d84..dbe10cb 100644 --- a/js/mp3-encoder.worker.js +++ b/js/mp3-encoder.worker.js @@ -2,35 +2,37 @@ import { FFmpeg } from '@ffmpeg/ffmpeg'; import { toBlobURL } from '@ffmpeg/util'; let ffmpeg = null; -let isLoaded = false; +let loadingPromise = null; async function loadFFmpeg() { - if (isLoaded) return; + if (loadingPromise) return loadingPromise; - ffmpeg = new FFmpeg(); - - ffmpeg.on('log', ({ message }) => { - self.postMessage({ type: 'log', message }); - }); - - ffmpeg.on('progress', ({ progress, time }) => { - self.postMessage({ - type: 'progress', - stage: 'encoding', - progress: progress * 100, - time + loadingPromise = (async () => { + ffmpeg = new FFmpeg(); + + ffmpeg.on('log', ({ message }) => { + self.postMessage({ type: 'log', message }); }); - }); + + ffmpeg.on('progress', ({ progress, time }) => { + self.postMessage({ + type: 'progress', + stage: 'encoding', + progress: progress * 100, + time + }); + }); + + self.postMessage({ type: 'progress', stage: 'loading', message: 'Loading FFmpeg...' }); + + const baseURL = 'https://unpkg.com/@ffmpeg/core@0.12.6/dist/esm'; + await ffmpeg.load({ + coreURL: await toBlobURL(`${baseURL}/ffmpeg-core.js`, 'text/javascript'), + wasmURL: await toBlobURL(`${baseURL}/ffmpeg-core.wasm`, 'application/wasm') + }); + })(); - self.postMessage({ type: 'progress', stage: 'loading', message: 'Loading FFmpeg...' }); - - const baseURL = 'https://unpkg.com/@ffmpeg/core@0.12.6/dist/esm'; - await ffmpeg.load({ - coreURL: await toBlobURL(`${baseURL}/ffmpeg-core.js`, 'text/javascript'), - wasmURL: await toBlobURL(`${baseURL}/ffmpeg-core.wasm`, 'application/wasm') - }); - - isLoaded = true; + return loadingPromise; } self.onmessage = async (e) => { @@ -41,29 +43,40 @@ self.onmessage = async (e) => { self.postMessage({ type: 'progress', stage: 'encoding', message: 'Encoding to MP3 320kbps...' }); - // Write input file to FFmpeg virtual filesystem - await ffmpeg.writeFile('input', new Uint8Array(audioData)); - - // Encode to MP3 with 320kbps CBR (FFmpeg auto-detects input format) - await ffmpeg.exec([ - '-i', 'input', - '-c:a', 'libmp3lame', - '-b:a', '320k', - '-ar', '44100', - 'output.mp3' - ]); - - self.postMessage({ type: 'progress', stage: 'finalizing', message: 'Finalizing MP3...' }); - - // Read output file - const data = await ffmpeg.readFile('output.mp3'); - const mp3Blob = new Blob([data.buffer], { type: 'audio/mpeg' }); - - // Cleanup - await ffmpeg.deleteFile('input'); - await ffmpeg.deleteFile('output.mp3'); - - self.postMessage({ type: 'complete', blob: mp3Blob }); + try { + // Write input file to FFmpeg virtual filesystem + await ffmpeg.writeFile('input', new Uint8Array(audioData)); + + // Encode to MP3 with 320kbps CBR, strip source metadata to avoid duplicate ID3 tags + await ffmpeg.exec([ + '-i', 'input', + '-map_metadata', '-1', + '-c:a', 'libmp3lame', + '-b:a', '320k', + '-ar', '44100', + 'output.mp3' + ]); + + self.postMessage({ type: 'progress', stage: 'finalizing', message: 'Finalizing MP3...' }); + + // Read output file - use Uint8Array directly to avoid extra bytes from ArrayBuffer + const data = await ffmpeg.readFile('output.mp3'); + const mp3Blob = new Blob([data], { type: 'audio/mpeg' }); + + self.postMessage({ type: 'complete', blob: mp3Blob }); + } finally { + // Always cleanup virtual filesystem files + try { + await ffmpeg.deleteFile('input'); + } catch { + // File may not exist if writeFile failed + } + try { + await ffmpeg.deleteFile('output.mp3'); + } catch { + // File may not exist if exec failed + } + } } catch (error) { self.postMessage({ type: 'error', message: error.message }); }