[RNMobile] Fix resize video after upload #30071

jhnstn · 2023-04-12T21:26:57Z

Fixes wordpress-mobile/gutenberg-mobile#5603
Depends on: WordPress/gutenberg#49929
The VP api will return a preview with default dimensions before it's fully processed the video. As a result the video will render cropped after uploading. It can take up to 4 seconds before the correct video dimensions are available.

This PR will attempt to fetch the completed preview.

Proposed changes:

Adds a retry flow to request the video preview until its ready to render

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Add a VideoPress block and choose a video that does not have a 16:9 aspect ratio e.g. something recorded in portrait.
After a few seconds the video should appear with the correct aspect ratio.
Save and reload the post
The video should load with the correct aspect ratio with a delay.

github-actions · 2023-04-12T21:27:58Z

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run

bin/jetpack-downloader test jetpack rnmobile/fix/resize-after-upload

to get started. More details: p9dueE-5Nn-p2

github-actions · 2023-04-12T21:28:26Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
⚠️ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

jhnstn · 2023-04-14T14:29:04Z

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js

@@ -17,6 +19,11 @@ import { usePreview } from '../../../../hooks/use-preview';
 import addTokenIntoIframeSource from '../../../../utils/add-token-iframe-source';
 import style from './style.scss';

+const VIDEO_PREVIEW_ATTEMPTS_LIMIT = 10;
+
+// The preview is ready when it has a height property


I considered moving this down to the usePreview hook but the web player doesn't check this explicitly. The polling end conditions are a bit more complicated . I'm not sure if the preview behavior is different on web but from my observations we need the preview to have an explicit height property before we can let the Sandbox component scale properly.

jhnstn · 2023-04-14T14:31:25Z

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js

+			}
+
+			// Avoid lapping any prior request
+			if ( isRequestingEmbedPreview ) {


I considered having the counter increment before this early return. I left it below since an ongoing request has not been evaluated and should not count as an attempt.

jhnstn · 2023-04-14T14:36:22Z

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js

+			// Invalidate the cache and wait for the next interval since the preview is not ready yet
+			setPreviewCheckAttempts( previewCheckAttempts + 1 );
+			invalidatePreview();
+		}, 1000 );


Web uses 2 seconds between the requests which we might want to follow. The setInterval will run immediately so we get a request at t=0, t=1, etc. I've noticed it takes about 2 attempts which in this set up it takes about a second to get a complete preview.
The other option could be to switch this to a setTimeout loop so we get requests at t=1, t=2, etc.

fluiddot · 2023-04-14T13:40:27Z

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js

+	const invalidatePreview = useCallback( () => {
+		invalidateResolution( 'getEmbedPreview', [ videoPressUrl ] );
+	}, [ videoPressUrl, invalidateResolution ] );


Not sure if we need to memoize this function, do we foresee any extra re-render by not using useCallback?

fluiddot · 2023-04-14T13:40:35Z

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js

+	const cancelPreviewCheckInterval = useCallback( () => {
+		clearInterval( previewCheckInterval.current );
+	}, [ previewCheckInterval ] );


Not sure if we need to memoize this function, do we foresee any extra re-render by not using useCallback?

fluiddot · 2023-04-14T17:27:03Z

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js

+		if ( ! isPlayerLoaded ) {
+			return;
+		}
+		previewCheckInterval.current = setInterval( () => {


I tested the case of loading a post with a VideoPress video and noticed that the interval is never canceled. As you can see in the video capture, previewCheckAttempts remains with value 1, and isRequestingEmbedPreview is always true although the preview is rendered.

ios-preview-interval.mp4

NOTE: The console log is called within the setInterval function.

I checked the value of isRequestingEmbedPreview in the render function and it changes to false while the interval function still gets true 🤔. @jhnstn let me know if you can reproduce this issue, thanks!

That's the same behavior I am seeing on Android after uploading. But I've haven't seen that on iOS or on Android with existing videos.

What's odd about this is that the embed endpoint for VideoPress should be returning a preview with height and width properties when opening a post with an existing video. I have not seen this before but good to know that this can happen.

I'm still not sure why isRequestingEmbedPreview getting stuck on true. I've tried changing the order of logic but nothing seems to change.

What's odd about this is that the embed endpoint for VideoPress should be returning a preview with height and width properties when opening a post with an existing video. I have not seen this before but good to know that this can happen.

Yep, I noticed in the response of the preview that the provider_name field is Embed handler which is the default response when it can't infer the provider. In fact, when the provider is not inferred is also when the width and height are empty. The second preview request responds provider_name with VideoPress and the proper dimensions. As I comment below, I think the fact that is not returning the dimensions is related to the effect and the hook's dependencies.

I'm still not sure why isRequestingEmbedPreview getting stuck on true. I've tried changing the order of logic but nothing seems to change.

I think this can be related to using setInterval within the useEffect and the hook's dependencies. I played around with this part and seems we could replace it with setTimeout:

diff --git forkSrcPrefix/projects/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js forkDstPrefix/projects/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js index 785230d0c32b6f497dd47431c7975511f20255c9..ebb1a3afcdc101c96a9ee9a91f82fa29357af766 100644 --- forkSrcPrefix/projects/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js +++ forkDstPrefix/projects/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js @@ -88,28 +88,21 @@ export default function Player( { isSelected, attributes } ) { // Fetch the preview until it's ready useEffect( () => { - // Wait for the client player to load - if ( ! isPlayerLoaded ) { + // Only check preview once it's available. + if ( ! isPlayerLoaded || isRequestingEmbedPreview ) { return; } - previewCheckInterval.current = setInterval( () => { - // if the preview is ready or we reached the max attempts, clear the interval - if ( isPreviewReady( preview ) || previewCheckAttempts > VIDEO_PREVIEW_ATTEMPTS_LIMIT ) { - return cancelPreviewCheckInterval(); - } - - // Avoid lapping any prior request - if ( isRequestingEmbedPreview ) { - return; - } - - // Invalidate the cache and wait for the next interval since the preview is not ready yet - setPreviewCheckAttempts( previewCheckAttempts + 1 ); - invalidatePreview(); - }, 1000 ); + + // Request another preview if it's not ready and haven't exceeded request attemtps. + if ( ! isPreviewReady( preview ) && previewCheckAttempts <= VIDEO_PREVIEW_ATTEMPTS_LIMIT ) { + previewCheckInterval.current = setTimeout( () => { + invalidatePreview(); + setPreviewCheckAttempts( previewCheckAttempts + 1 ); + }, 1000 ); + } return cancelPreviewCheckInterval; - }, [ preview, isPlayerLoaded ] ); + }, [ preview, isPlayerLoaded, isRequestingEmbedPreview, previewCheckAttempts ] ); const onSandboxMessage = useCallback( message => { if ( message.event === 'videopress_loading_state' && message.state === 'loaded' ) {

Let me know if you'd like to proceed with this option.

That's the same behavior I am seeing on Android after uploading. But I've haven't seen that on iOS or on Android with existing videos.

I'm wondering if the problem on Android has to do with the caching layer on the api-fetch handler (reference). On Android GET requests are cached in the host app by default unless it's specified, maybe for the previews we should disable it.

I played around with this part and seems we could replace it with setTimeout:

Yeah I figured out that setTimeout would not cache the preview values. I didn't bother continuing on that since it didn't do much on Android.

Thanks for the reference to the caching setup . I was looking into the native version of the api-fetch component but didn't know about that set up fie. Seems like this can be fixed with setTimeout and adjusting the Android request caching.

jhnstn · 2023-04-14T23:18:53Z

@fluiddot I tried modifying the loop logic but found invalidateResolution( 'getEmbedPreview', [ videoPressUrl ] ); wasn't doing too much. It appears the request is getting cached somewhere. I had thought about this situation before I found invalidateResolution being used in the web player but hoped I could just leverage the resolution invalidation.

I went back to my original thought and tried adding a counter as a query param to trick the app into making a new request. That's working although I feel it's a bit hacky.

I'm still not sure why you were seeing the preview come back incomplete on iOS. As I mentioned I have only seen issues on Android.

I can follow up on this approach with the VideoPress devs on Monday. This is working but I'm not super happy with it.

Revert attempt to break cache via url params

fluiddot

LGTM 🎊 !

iOS

Works as expected ✅

Android

When uploading a video from the device on Android, I'm still experiencing the issue I mentioned here. For some reason, the preview is not identified as VideoPress (i.e. its provider_name attribute is VideoPress) but as a generic embed (i.e. the provider_name is Embed handler). This results in the preview being cropped.

Preview object:

{
  "html": "<iframe title='VideoPress Video Player' aria-label='VideoPress Video Player' width='600' height='338' src='https://video.wordpress.com/embed/<VIDEOPRESS_GUID>?cover=1&amp;preloadContent=metadata&amp;hd=1&amp;metadata_token=<TOKEN>' frameborder='0' allowfullscreen data-resize-to-parent=\"true\"  allow='clipboard-write'></iframe><script src='https://v0.wordpress.com/js/next/videopress-iframe.js?m=<ID>'></script>",
  "provider_name": "Embed Handler",
  "scripts": [
    "/wp-content/js/devicepx.js",
    "/wp-content/js/mobile-useragent-info.js",
    "https://s0.wp.com/wp-content/plugins/video/assets/js/videojs/videopress-iframe-api.js",
    "/wp-content/mu-plugins/subscriptions/follow.js"
  ]
}

android-videopress.mp4

Tested on Samsung Galaxy S20 FE 5G (Android 13).

However, this can't be reproduced when applying the changes from WordPress/gutenberg#49929, so I'd consider this issue as fixed ✅.

Minor issues

I noticed that during the video conversion, the block has an extra space at the bottom (see attached screenshot).

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js

…deo/components/player/index.native.js Co-authored-by: Carlos Garcia <fluiddot@gmail.com>

SiobhyB

🚀 🚢

jhnstn marked this pull request as draft April 12, 2023 21:27

jhnstn requested review from fluiddot and SiobhyB April 12, 2023 21:27

jhnstn added the [Package] VideoPress label Apr 12, 2023

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Apr 12, 2023

jhnstn added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Apr 12, 2023

jhnstn force-pushed the rnmobile/fix/resize-after-upload branch from af17d54 to bef9246 Compare April 13, 2023 14:57

jhnstn marked this pull request as ready for review April 13, 2023 17:22

jhnstn changed the title ~~Rnmobile/fix/resize after upload~~ [RNMobile] Fix resize video after upload Apr 13, 2023

jhnstn added 3 commits April 13, 2023 16:39

[not verified] Simplify how the embed is rendered

4c51bb5

[not verified] Continue requesting the preview until its ready to render

937ea9d

[not verified] changelog

213f2c5

jhnstn force-pushed the rnmobile/fix/resize-after-upload branch from bef9246 to 213f2c5 Compare April 14, 2023 13:50

jhnstn commented Apr 14, 2023

View reviewed changes

fluiddot reviewed Apr 14, 2023

View reviewed changes

Add attempts to VP url to force a request

850139d

jhnstn added 2 commits April 17, 2023 22:04

Update preview update flow

ebfe916

Revert attempt to break cache via url params

fix loading copy

26d8150

jhnstn requested a review from fluiddot April 19, 2023 17:34

fluiddot previously approved these changes Apr 20, 2023

View reviewed changes

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js Outdated Show resolved Hide resolved

jhnstn and others added 2 commits April 20, 2023 12:35

Merge branch 'trunk' into rnmobile/fix/resize-after-upload

5d3d8b1

Update projects/packages/videopress/src/client/block-editor/blocks/vi…

c7c0e59

…deo/components/player/index.native.js Co-authored-by: Carlos Garcia <fluiddot@gmail.com>

jhnstn dismissed fluiddot’s stale review via c7c0e59 April 20, 2023 16:39

jhnstn enabled auto-merge (squash) April 20, 2023 16:54

SiobhyB approved these changes Apr 20, 2023

View reviewed changes

jhnstn merged commit 3e1b213 into trunk Apr 20, 2023

jhnstn deleted the rnmobile/fix/resize-after-upload branch April 20, 2023 17:02

github-actions bot removed the [Status] In Progress label Apr 20, 2023

fluiddot mentioned this pull request Apr 21, 2023

Enable VideoPress block on iOS wordpress-mobile/gutenberg-mobile#5692

Merged

2 tasks

zinigor added this to the Jetpack/12.1 milestone Apr 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Fix resize video after upload #30071

[RNMobile] Fix resize video after upload #30071

jhnstn commented Apr 12, 2023 •

edited

Loading

github-actions bot commented Apr 12, 2023 •

edited

Loading

github-actions bot commented Apr 12, 2023 •

edited

Loading

jhnstn Apr 14, 2023

jhnstn Apr 14, 2023

jhnstn Apr 14, 2023

fluiddot Apr 14, 2023

fluiddot Apr 14, 2023

fluiddot Apr 14, 2023 •

edited

Loading

fluiddot Apr 14, 2023

jhnstn Apr 14, 2023 •

edited

Loading

jhnstn Apr 14, 2023

fluiddot Apr 17, 2023

fluiddot Apr 17, 2023

jhnstn Apr 17, 2023 •

edited

Loading

jhnstn commented Apr 14, 2023

fluiddot left a comment

SiobhyB left a comment

[RNMobile] Fix resize video after upload #30071

[RNMobile] Fix resize video after upload #30071

Conversation

jhnstn commented Apr 12, 2023 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Apr 12, 2023 • edited Loading

github-actions bot commented Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluiddot Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhnstn Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhnstn Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

jhnstn commented Apr 14, 2023

fluiddot left a comment

Choose a reason for hiding this comment

iOS

Android

Minor issues

SiobhyB left a comment

Choose a reason for hiding this comment

jhnstn commented Apr 12, 2023 •

edited

Loading

github-actions bot commented Apr 12, 2023 •

edited

Loading

github-actions bot commented Apr 12, 2023 •

edited

Loading

fluiddot Apr 14, 2023 •

edited

Loading

jhnstn Apr 14, 2023 •

edited

Loading

jhnstn Apr 17, 2023 •

edited

Loading