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
This commit is contained in:
gpulch 2026-02-23 10:49:05 +01:00
parent 8a17bddbc3
commit cde7080052
4 changed files with 98 additions and 59 deletions

View file

@ -10,7 +10,7 @@ import { trackDateSettings } from './storage.js';
import { APICache } from './cache.js'; import { APICache } from './cache.js';
import { addMetadataToAudio } from './metadata.js'; import { addMetadataToAudio } from './metadata.js';
import { DashDownloader } from './dash-downloader.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'; export const DASH_MANIFEST_UNAVAILABLE_CODE = 'DASH_MANIFEST_UNAVAILABLE';
const TIDAL_V2_TOKEN = 'txNoH4kkV41MfH25'; const TIDAL_V2_TOKEN = 'txNoH4kkV41MfH25';
@ -1137,7 +1137,7 @@ export class LosslessAPI {
// Convert to MP3 320kbps if requested // Convert to MP3 320kbps if requested
if (quality === 'MP3_320') { if (quality === 'MP3_320') {
try { try {
blob = await encodeToMp3(blob, onProgress); blob = await encodeToMp3(blob, onProgress, options.signal);
} catch (encodingError) { } catch (encodingError) {
if (onProgress) { if (onProgress) {
onProgress({ onProgress({
@ -1145,7 +1145,7 @@ export class LosslessAPI {
message: `Encoding failed: ${encodingError.message}`, message: `Encoding failed: ${encodingError.message}`,
}); });
} }
throw new Error(`MP3 encoding failed: ${encodingError.message}`); throw encodingError;
} }
} }
@ -1176,7 +1176,7 @@ export class LosslessAPI {
throw error; throw error;
} }
console.error('Download failed:', 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; throw error;
} }
if (error.message === RATE_LIMIT_ERROR_MESSAGE) { if (error.message === RATE_LIMIT_ERROR_MESSAGE) {

View file

@ -29,7 +29,6 @@ async function writeID3v2Tag(mp3Blob, metadata, coverBlob = null) {
const year = new Date(metadata.album.releaseDate).getFullYear(); const year = new Date(metadata.album.releaseDate).getFullYear();
if (!Number.isNaN(year) && Number.isFinite(year)) { if (!Number.isNaN(year) && Number.isFinite(year)) {
frames.push(createTextFrame('TYER', year.toString())); frames.push(createTextFrame('TYER', year.toString()));
frames.push(createTextFrame('TDRC', year.toString()));
} }
} }

View file

@ -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(); const audioData = await audioBlob.arrayBuffer();
return new Promise((resolve, reject) => { return new Promise((resolve, reject) => {
const worker = new Worker(new URL('./mp3-encoder.worker.js', import.meta.url), { type: 'module' }); 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) => { worker.onmessage = (e) => {
const { type, blob, message, stage, progress } = e.data; const { type, blob, message, stage, progress } = e.data;
if (type === 'complete') { if (type === 'complete') {
if (signal) signal.removeEventListener('abort', abortHandler);
worker.terminate(); worker.terminate();
resolve(blob); resolve(blob);
} else if (type === 'error') { } else if (type === 'error') {
if (signal) signal.removeEventListener('abort', abortHandler);
worker.terminate(); worker.terminate();
reject(new Error(message)); reject(new MP3EncodingError(message));
} else if (type === 'progress' && onProgress) { } else if (type === 'progress' && onProgress) {
onProgress({ stage, message, progress }); onProgress({ stage, message, progress });
} else if (type === 'log') { } else if (type === 'log') {
@ -21,8 +45,9 @@ async function encodeToMp3Worker(audioBlob, onProgress = null) {
}; };
worker.onerror = (error) => { worker.onerror = (error) => {
if (signal) signal.removeEventListener('abort', abortHandler);
worker.terminate(); worker.terminate();
reject(new Error('Worker failed: ' + error.message)); reject(new MP3EncodingError('Worker failed: ' + error.message));
}; };
// Transfer audio data to worker // 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 { try {
// Use Web Worker for non-blocking FFmpeg encoding // Use Web Worker for non-blocking FFmpeg encoding
if (typeof Worker !== 'undefined') { 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) { } catch (error) {
console.error('MP3 encoding failed:', error); console.error('MP3 encoding failed:', error);
throw new Error('Failed to encode MP3: ' + error.message); throw error;
} }
} }
export { MP3EncodingError };

View file

@ -2,35 +2,37 @@ import { FFmpeg } from '@ffmpeg/ffmpeg';
import { toBlobURL } from '@ffmpeg/util'; import { toBlobURL } from '@ffmpeg/util';
let ffmpeg = null; let ffmpeg = null;
let isLoaded = false; let loadingPromise = null;
async function loadFFmpeg() { async function loadFFmpeg() {
if (isLoaded) return; if (loadingPromise) return loadingPromise;
ffmpeg = new FFmpeg(); loadingPromise = (async () => {
ffmpeg = new FFmpeg();
ffmpeg.on('log', ({ message }) => {
self.postMessage({ type: 'log', message }); ffmpeg.on('log', ({ message }) => {
}); self.postMessage({ type: 'log', message });
ffmpeg.on('progress', ({ progress, time }) => {
self.postMessage({
type: 'progress',
stage: 'encoding',
progress: progress * 100,
time
}); });
});
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...' }); return loadingPromise;
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;
} }
self.onmessage = async (e) => { self.onmessage = async (e) => {
@ -41,29 +43,40 @@ self.onmessage = async (e) => {
self.postMessage({ type: 'progress', stage: 'encoding', message: 'Encoding to MP3 320kbps...' }); self.postMessage({ type: 'progress', stage: 'encoding', message: 'Encoding to MP3 320kbps...' });
// Write input file to FFmpeg virtual filesystem try {
await ffmpeg.writeFile('input', new Uint8Array(audioData)); // 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([ // Encode to MP3 with 320kbps CBR, strip source metadata to avoid duplicate ID3 tags
'-i', 'input', await ffmpeg.exec([
'-c:a', 'libmp3lame', '-i', 'input',
'-b:a', '320k', '-map_metadata', '-1',
'-ar', '44100', '-c:a', 'libmp3lame',
'output.mp3' '-b:a', '320k',
]); '-ar', '44100',
'output.mp3'
self.postMessage({ type: 'progress', stage: 'finalizing', message: 'Finalizing MP3...' }); ]);
// Read output file self.postMessage({ type: 'progress', stage: 'finalizing', message: 'Finalizing MP3...' });
const data = await ffmpeg.readFile('output.mp3');
const mp3Blob = new Blob([data.buffer], { type: 'audio/mpeg' }); // Read output file - use Uint8Array directly to avoid extra bytes from ArrayBuffer
const data = await ffmpeg.readFile('output.mp3');
// Cleanup const mp3Blob = new Blob([data], { type: 'audio/mpeg' });
await ffmpeg.deleteFile('input');
await ffmpeg.deleteFile('output.mp3'); self.postMessage({ type: 'complete', blob: mp3Blob });
} finally {
self.postMessage({ type: 'complete', blob: mp3Blob }); // 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) { } catch (error) {
self.postMessage({ type: 'error', message: error.message }); self.postMessage({ type: 'error', message: error.message });
} }