Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/upload-media/CHANGELOG.md

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to merge this PR, but it's a hassle to find out which PR actually made the change, as the recent entries in this changelog file link to issues rather than PRs. It would probably be better to fix this on both the trunk and wp/7.1 branches.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-hamano so the changelog entries should point to the PRs merged instead of the issue? I can go back and fix all of the client side media ones if thats helpful I likely did them all the same way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the changelog entries should point to the PRs merged instead of the issue?

There are no strict rules, but I believe changelog entries have been that way in the past. However, it's not urgent, so I think it's fine to correct it later 👍

Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@

### Enhancements

- Honor the `image_strip_meta` and `image_max_bit_depth` filters for client-side processed images via the new `imageStripMeta` and `imageMaxBitDepth` settings, carried in the REST API root index ([#80216](https://github.com/WordPress/gutenberg/issues/80216)).
- Honor the `image_strip_meta` and `image_max_bit_depth` filters for client-side processed images via the new `imageStripMeta` and `imageMaxBitDepth` settings, carried in the REST API root index ([#80216](https://github.com/WordPress/gutenberg/issues/80216)).
- Animated GIF to video conversion is now abandoned after a timeout (default 30 seconds) and skipped entirely for GIFs whose total decoded pixels (width × height × frame count) exceed a budget. In both cases the original GIF upload is kept and no error is surfaced; a `SCRIPT_DEBUG` diagnostic explains the skip. Both knobs are configurable via the `TranscodeGif` operation args ([#80376](https://github.com/WordPress/gutenberg/issues/80376)).

### Bug Fixes

- `cancelItem` no longer awaits the best-effort worker cancellation calls. A busy vips worker is synchronously blocked inside a wasm call and cannot answer the cancellation RPC until every operation already queued in the worker finishes, which for a large animated GIF left cancelled items stuck in the queue - and the parent attachment unfinalized - for minutes ([#80376](https://github.com/WordPress/gutenberg/issues/80376)).
- Pass `isTransportOnly: true` to the `mediaUpload` setting when the queue uploads a file, so consumers that manage the upload lifecycle themselves (progress tracking, save locking) don't handle the same file twice ([#80369](https://github.com/WordPress/gutenberg/issues/80369)).

## 0.36.0 (2026-07-14)
Expand Down
14 changes: 11 additions & 3 deletions packages/upload-media/src/store/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,23 @@ export function cancelItem( id: QueueItemId, error: Error, silent = false ) {
* best-effort cleanup: when the worker has crashed, the cancel call
* itself rejects, and that must not abort the cancellation flow —
* onError, item removal, and worker teardown below still need to run.
*
* Deliberately NOT awaited: a busy worker is synchronously blocked
* inside a wasm call and cannot answer the cancellation RPC until
* the current operation — and every operation already queued behind
* it — finishes. For a large animated GIF that is minutes, and
* awaiting here would leave the cancelled item in the queue (gating
* the parent's finalization) that whole time. Nothing below depends
* on the result.
*/
await vipsCancelOperations( id ).catch( () => {} );
vipsCancelOperations( id ).catch( () => {} );

/*
* Cancel any ongoing GIF-to-video conversion for this item so a
* cancelled upload does not leave the encoder running off-thread.
* Best-effort for the same reason as above.
* Best-effort and fire-and-forget for the same reasons as above.
*/
await cancelGifToVideoOperations( id ).catch( () => {} );
cancelGifToVideoOperations( id ).catch( () => {} );

if ( ! silent ) {
const { onError } = item;
Expand Down
46 changes: 44 additions & 2 deletions packages/upload-media/src/store/private-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { getImageDimensions } from '../get-image-dimensions';
import { CLIENT_SIDE_SUPPORTED_MIME_TYPES, HEIC_MIME_TYPES } from './constants';
import { StubFile } from '../stub-file';
import { ErrorCode, UploadError } from '../upload-error';
import { measure } from './utils/debug-logger';
import { debug, measure } from './utils/debug-logger';
import {
vipsResizeImage,
vipsRotateImage,
Expand All @@ -42,6 +42,8 @@ import {
} from './utils';
import {
convertGifToVideo,
isConversionTimeoutError,
isSizeLimitConversionError,
isUnsupportedConversionError,
terminateVideoConversionWorker,
} from './utils/video-conversion';
Expand Down Expand Up @@ -442,6 +444,10 @@ export function processItem( id: QueueItemId ) {
operation,
} );

debug(
`Starting operation ${ operation } for ${ item.file.name } (item ${ item.id })`
);

switch ( operation ) {
case OperationType.Prepare:
dispatch.prepareItem( item.id );
Expand Down Expand Up @@ -1407,7 +1413,11 @@ export function transcodeGifItem(
const file = await convertGifToVideo(
item.id,
gifFile,
outputMimeType
outputMimeType,
{
timeout: args?.timeout,
maxTotalPixels: args?.maxTotalPixels,
}
);

// Hand the transcoded video to the next Upload op as the
Expand Down Expand Up @@ -1455,13 +1465,45 @@ export function transcodeGifItem(
// create an `animated_video` meta entry pointing at the GIF
// itself — meaningless.
if ( isUnsupportedConversionError( error ) ) {
/*
* A GIF skipped for exceeding the total-pixel budget is a
* graceful outcome like any other unsupported conversion,
* but worth a SCRIPT_DEBUG diagnostic so developers testing
* large GIFs understand why no companion video was produced.
*/
if ( isSizeLimitConversionError( error ) ) {
debug(
`Skipping GIF to video conversion: ${
error instanceof Error ? error.message : error
}`
);
}
dispatch.cancelItem(
id,
new Error( 'Animated GIF conversion unsupported' ),
true
);
return;
}
/*
* The conversion ran past the allowed time and was abandoned:
* the GIF attachment stands alone, which is exactly what the
* user uploaded. SCRIPT_DEBUG diagnostic only, no user-facing
* error.
*/
if ( isConversionTimeoutError( error ) ) {
debug(
`GIF to video conversion timed out; keeping the original GIF only: ${
error instanceof Error ? error.message : error
}`
);
dispatch.cancelItem(
id,
new Error( 'Animated GIF conversion timed out' ),
true
);
return;
}
// Real engine failure. The parent GIF attachment is fine —
// the user just won't get a companion video. Log the cause
// for debuggability and silently cancel the sideload; we do
Expand Down
39 changes: 39 additions & 0 deletions packages/upload-media/src/store/test/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,45 @@ describe( 'actions', () => {
).toHaveLength( 0 );
} );

it( 'removes the item even when the workers never answer the cancel calls', async () => {
/*
* A busy vips worker is synchronously blocked inside a wasm call
* and cannot answer the cancellation RPC until the current
* operation - and every operation already queued behind it -
* finishes, which for a large animated GIF takes minutes. The
* worker cancels are best-effort cleanup; cancelItem must not
* block on them, or the cancelled item lingers in the queue and
* gates the parent's finalization the whole time.
*/
( vipsCancelOperations as jest.Mock ).mockImplementationOnce(
() => new Promise( () => {} )
);
( cancelGifToVideoOperations as jest.Mock ).mockImplementationOnce(
() => new Promise( () => {} )
);

const onError = jest.fn();
unlock( registry.dispatch( uploadStore ) ).addItem( {
file: jpegFile,
onError,
} );
const item = unlock(
registry.select( uploadStore )
).getAllItems()[ 0 ];

await registry
.dispatch( uploadStore )
.cancelItem( item.id, new Error( 'Test error' ) );

expect( vipsCancelOperations ).toHaveBeenCalledWith( item.id );
expect( onError ).toHaveBeenCalledWith(
expect.objectContaining( { message: 'Test error' } )
);
expect(
unlock( registry.select( uploadStore ) ).getAllItems()
).toHaveLength( 0 );
} );

describe( 'parent cancellation when child sideload fails', () => {
// Helpers used by every scenario below. Set up a parent that
// has finished its primary upload (so it has an attachment.id),
Expand Down
87 changes: 85 additions & 2 deletions packages/upload-media/src/store/test/private-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ jest.mock( '../utils/video-conversion', () => {
cancelGifToVideoOperations: jest.fn(),
terminateVideoConversionWorker: jest.fn(),
isUnsupportedConversionError: actual.isUnsupportedConversionError,
isSizeLimitConversionError: actual.isSizeLimitConversionError,
isConversionTimeoutError: actual.isConversionTimeoutError,
};
} );

Expand Down Expand Up @@ -724,17 +726,22 @@ describe( 'private actions', () => {
}

let consoleError;
let consoleDebug;

beforeEach( () => {
convertGifToVideo.mockReset();
createBlobURL.mockClear();
consoleError = jest
.spyOn( console, 'error' )
.mockImplementation( () => {} );
consoleDebug = jest
.spyOn( console, 'debug' )
.mockImplementation( () => {} );
} );

afterEach( () => {
consoleError.mockRestore();
consoleDebug.mockRestore();
} );

it( 'hands the transcoded video to the next Upload via finishOperation', async () => {
Expand All @@ -752,7 +759,8 @@ describe( 'private actions', () => {
expect( convertGifToVideo ).toHaveBeenCalledWith(
'gif-1',
gifFile,
'video/mp4'
'video/mp4',
{ timeout: undefined, maxTotalPixels: undefined }
);
// Sideload context: no CacheBlobUrl, no attachment URL update -
// the parent GIF attachment already owns the block's URL.
Expand Down Expand Up @@ -812,7 +820,30 @@ describe( 'private actions', () => {
expect( convertGifToVideo ).toHaveBeenCalledWith(
'gif-1',
gifFile,
'video/mp4'
'video/mp4',
{ timeout: undefined, maxTotalPixels: undefined }
);
} );

it( 'passes timeout and maxTotalPixels operation args through', async () => {
convertGifToVideo.mockResolvedValue(
new File( [ 'webm' ], 'animation.webm', {
type: 'video/webm',
} )
);
const { select, dispatch } = buildArgs();

await transcodeGifItem( 'gif-1', {
outputFormat: 'webm',
timeout: 5000,
maxTotalPixels: 1_000_000,
} )( { select, dispatch } );

expect( convertGifToVideo ).toHaveBeenCalledWith(
'gif-1',
gifFile,
'video/webm',
{ timeout: 5000, maxTotalPixels: 1_000_000 }
);
} );

Expand All @@ -839,6 +870,58 @@ describe( 'private actions', () => {
expect( dispatch.addSideloadItem ).not.toHaveBeenCalled();
} );

it( 'logs a SCRIPT_DEBUG diagnostic and silently cancels when the GIF exceeds the conversion size limit', async () => {
// An over-budget GIF is a graceful skip like any Unsupported
// outcome, but the skip is logged (under SCRIPT_DEBUG, true in
// this test env) so developers testing large GIFs understand
// why no companion video was produced.
convertGifToVideo.mockRejectedValue(
new Error(
'Unsupported: GIF exceeds maximum conversion size (5000x5000 x 100 frames = 2500000000 pixels; limit is 300000000)'
)
);
const { select, dispatch } = buildArgs();

await transcodeGifItem( 'gif-1' )( { select, dispatch } );

expect( dispatch.finishOperation ).not.toHaveBeenCalled();
expect( dispatch.cancelItem ).toHaveBeenCalledTimes( 1 );
const [ cancelledId, , silent ] =
dispatch.cancelItem.mock.calls[ 0 ];
expect( cancelledId ).toBe( 'gif-1' );
expect( silent ).toBe( true );
expect( consoleDebug ).toHaveBeenCalledWith(
expect.stringContaining( 'exceeds maximum conversion size' )
);
expect( consoleError ).not.toHaveBeenCalled();
expect( dispatch.addSideloadItem ).not.toHaveBeenCalled();
} );

it( 'logs a SCRIPT_DEBUG diagnostic and silently cancels when the conversion times out', async () => {
// The conversion was abandoned after the timeout; the GIF
// attachment stands alone. No user-facing error, but the
// timeout is logged (under SCRIPT_DEBUG) for debuggability.
convertGifToVideo.mockRejectedValue(
new Error( 'GIF to video conversion timed out after 30000ms' )
);
const { select, dispatch } = buildArgs();

await transcodeGifItem( 'gif-1' )( { select, dispatch } );

expect( dispatch.finishOperation ).not.toHaveBeenCalled();
expect( dispatch.cancelItem ).toHaveBeenCalledTimes( 1 );
const [ cancelledId, error, silent ] =
dispatch.cancelItem.mock.calls[ 0 ];
expect( cancelledId ).toBe( 'gif-1' );
expect( error.message ).toMatch( /timed out/i );
expect( silent ).toBe( true );
expect( consoleDebug ).toHaveBeenCalledWith(
expect.stringContaining( 'timed out' )
);
expect( consoleError ).not.toHaveBeenCalled();
expect( dispatch.addSideloadItem ).not.toHaveBeenCalled();
} );

it( 'silently cancels the sideload on a hard transcoding failure', async () => {
// A real engine failure is also not user-actionable on what the
// user thinks of as an image upload: log the cause, cancel
Expand Down
12 changes: 12 additions & 0 deletions packages/upload-media/src/store/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,18 @@ export interface OperationArgs {
[ OperationType.TranscodeGif ]: {
/** Video output format: 'mp4' or 'webm'. */
outputFormat: 'mp4' | 'webm';
/**
* Time in milliseconds before the conversion is abandoned and only
* the original GIF is kept. `0` disables the timeout. Defaults to
* 30 seconds.
*/
timeout?: number;
/**
* Budget for total decoded pixels (width × height × frame count)
* beyond which conversion is not attempted. `0` disables the check.
* Defaults to the `@wordpress/video-conversion` package default.
*/
maxTotalPixels?: number;
};
}

Expand Down
Loading
Loading