[RNMobile] Initial support for VideoPress v5 #35637

SiobhyB · 2024-02-13T15:28:30Z

Fixes:

[GlobalStep] Videos added via block editor do not show a thumbnail when previewed or published. wordpress-mobile/gutenberg-mobile#1909
Video Block: "Play inline" setting doesn't get saved wordpress-mobile/gutenberg-mobile#1721
[iOS] Video Block doesn't play MP4 wordpress-mobile/gutenberg-mobile#1998
Video doesn't play in browser when uploaded from the app. wordpress-mobile/gutenberg-mobile#3292
Video block: Videos can't be played in private sites wordpress-mobile/gutenberg-mobile#5497

Related PRs

Gutenberg: RNMobile: Add new Video block capability WordPress/gutenberg#59144
Gutenberg Mobile: Initial support for VideoPress v5 wordpress-mobile/gutenberg-mobile#6634
Android: Initial support for VideoPress v5 wordpress-mobile/WordPress-Android#20181
iOS: Initial support for VideoPress v5 wordpress-mobile/WordPress-iOS#22602

Proposed changes:

The same approach followed by Carlos in #29256 is used, except the changes apply to both platforms and a different end point is utilised. The main changes are:

3b4fe9b: Enabling the edit component for VideoPress.
ea3a0a9: Duplicating the core Video block's code.
f5e9233: Edit the core Video block's code, with the main change being fetching the VideoPress metadata.

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

N/A.

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

No, it does not.

Testing instructions:

Note, installable builds have been generated for both platforms and linked in the related PRs section for ease testing.

Testing environments:

The changes in this PR will affect WordPress.com users who use the video block in the app, and following environments will need to be tested to ensure everything works as expected:

Public simple WordPress.com site.
Public atomic WordPress.com site.
Private simple WordPress.com site.
Private atomic WordPress.com site.

Note, self-hosted sites (whether Jetpack-connected or not) should continue to use the core video block.

Verify fix for videos not displaying thumbnails when previewed/published and not playing in the browser when uploaded via the app

Add a new video block to a post.
Either take a video using your device or upload a video from your device's library.
After the video's upload, preview the post.
Verify the video's thumbnail displays and that it can be played.
Go ahead to publish the post to further verify that the video can be played in the browser.

Verify fix for plays inline setting not saving

Either add a video block to a post or navigate to an existing video block.
Open the settings panel.
Toggle a few different settings, including play inline.
Save your post then return to the video block's settings.
Verify all of your chosen settings were saved, including play inline.

Verify improvement when working between the web and the app

Add a video block via the app and save the post it's added to.
Navigate to the saved post in a web browser, make some changes to the block's settings or caption, and save.
Return to the post in the app, verify that it displays as expected and that any changed settings are saved.

Before	After

Further test for regressions

As this PR introduces a fairly large change to the way video block's work for WordPress.com sites, it'd be ideal to also go through the video block test cases to ensure there are no regressions.

With this commit, the main Video block files have been duplicated. They will be iterated on in subsequent commits and serve the basis of the changes we'll make to support VideoPress v5.

The Video block's is refactored to fetch VideoPress metadata, therefore enabling support for v5.

github-actions · 2024-02-13T15:29:27Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the rnmobile/support-for-videopress-v5 branch.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack rnmobile/support-for-videopress-v5

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2024-02-13T15:29:29Z

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.
🔴 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 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 reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

WordPress.com Simple releases happen daily.
WoA releases happen weekly.
Releases to self-hosted sites happen monthly. The next release is scheduled for March 5, 2024 (scheduled code freeze on March 4, 2024).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

SiobhyB · 2024-02-13T23:04:28Z

projects/plugins/jetpack/extensions/blocks/videopress/edit-common-settings.native.js

+			loop: toggleAttribute( 'loop' ),
+			muted: toggleAttribute( 'muted' ),
+			controls: toggleAttribute( 'controls' ),
+			playsinline: toggleAttribute( 'playsinline' ),


Note, the changed capitalisation of playsinline (previously playsInline) is intended, it's necessary to fix a bug with that setting. (See this relevant web change for an example of a change to fix a similar bug.)

SiobhyB · 2024-02-13T23:07:37Z

projects/plugins/jetpack/extensions/blocks/videopress/style.native.scss

I considered converting this to BEM CSS and also making some changes to the class function, but ultimately decided to keep aligning as much as possible with the video block's code, to hopefully make any future changes as smooth as possible. Interested to hear if others feel it'd be better to approach this differently, though!

SiobhyB · 2024-02-13T23:11:42Z

projects/plugins/jetpack/extensions/blocks/videopress/edit.native.js

+		// Private videos will open in the player for logged in users with the WordPress.com URL.
+		// However, they'll still display blank in the editor.
+		// TODO: We need to iterate so that private videos display as expected.
+		return metadata.is_private


I followed the function here to determine the best URL for each case:

jetpack/projects/packages/videopress/src/client/lib/url/index.ts

Lines 174 to 180 in eaddb2f

export function getVideoUrlBasedOnPrivacy( guid: VideoGUID, isPrivate: boolean ) {

if ( isPrivate ) {

return `https://video.wordpress.com/v/${ guid }`;

}

return `https://videopress.com/v/${ guid }`;

}

I went with this approach after being unsuccessful trying to get private videos to play in the editor on Android. I hope we can land this as a first iteration and fix the issues with private videos separately.

fluiddot · 2024-02-15T17:31:45Z

@SiobhyB I'm encountering a crash on the iOS app when uploading a video or adding a video from the media library. I could reproduce it with both iOS simulator and actual device. However, it only happens when using a local build, in the wordpress-mobile/WordPress-iOS#22602 (comment) can't be reproduced. I'm curious about what might be the difference and if it's an actual crash that users would encounter. I checked the same flow using trunk and it's working as expected 🤔.

ios-upload-video-crash.mp4

Here's the stack trace:

Exception Type:  EXC_BAD_ACCESS (SIGKILL)
Exception Subtype: KERN_INVALID_ADDRESS at 0x000000000000001e
Exception Codes: 0x0000000000000001, 0x000000000000001e

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib               	       0x1800362a8 object_isClass + 16
1   Foundation                    	       0x180d45370 KVO_IS_RETAINING_ALL_OBSERVERS_OF_THIS_OBJECT_IF_IT_CRASHES_AN_OBSERVER_WAS_OVERRELEASED_OR_SMASHED + 72
2   Foundation                    	       0x180d41cd4 NSKeyValueWillChangeWithPerThreadPendingNotifications + 284
3   AVFCore                       	       0x1a97316a0 __31-[AVPlayerItem _updateTimebase]_block_invoke_2 + 156
4   libdispatch.dylib             	       0x1801424f4 _dispatch_call_block_and_release + 24
5   libdispatch.dylib             	       0x180143d3c _dispatch_client_callout + 16
6   libdispatch.dylib             	       0x180152b24 _dispatch_main_queue_drain + 1272
7   libdispatch.dylib             	       0x18015261c _dispatch_main_queue_callback_4CF + 40
8   CoreFoundation                	       0x1803c61b4 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12
9   CoreFoundation                	       0x1803c08cc __CFRunLoopRun + 1936
10  CoreFoundation                	       0x1803bfd28 CFRunLoopRunSpecific + 572
11  GraphicsServices              	       0x18986ebc0 GSEventRunModal + 160
12  UIKitCore                     	       0x11c01bfdc -[UIApplication _run] + 868
13  UIKitCore                     	       0x11c01fc54 UIApplicationMain + 124
14  Jetpack                       	       0x1059298bc main + 540 (main.swift:7)
15  dyld_sim                      	       0x10ed61558 start_sim + 20
16  dyld                          	       0x10efda0e0 start + 2360

SiobhyB · 2024-02-15T18:10:15Z

@fluiddot, thank you for testing! I see the stack trace is very similar to the crash reported in wordpress-mobile/WordPress-iOS#20882, which leads back to a crash in react-native-video. I haven't been able to reproduce locally as of yet, but will keep looking into this tomorrow.

The strings used in the block's settings have been tweaked to match the web, so that translations can be reused.

Addresses feedback here: #35637 (comment)

derekblank · 2024-02-23T06:29:04Z

I tested replicating some of the scenarios from this comment:

❌ Inconsistent UI when retrying a failed upload
I noticed that retrying a failed upload results in an inconsistent UI (see attached video capture). Not that the progress bar and loading indicator are displayed with the error message "Failed to insert media".

I believe this may be coming from this logic:

jetpack/projects/plugins/jetpack/extensions/blocks/videopress/edit.native.js

Lines 369 to 377 in f1ae7fb

    
           { videoContainerHeight > 0 && 
        
           	( isUploadInProgress || isLoadingMetadata ? ( 
        
           		<ActivityIndicator /> 
        
           	) : ( 
        
           		iconContainer 
        
           	) ) } 
        
           { isUploadFailed && ( 
        
           	<Text style={ style.uploadFailedText }>{ retryMessage }</Text> 
        
           ) }

When the device goes offline, the upload is marked failed from the host apps, and the "Failed to Upload" retryMessage appears. (David and I wrestled with this a bit when working on the Upload Offline UX work.) Although we set isUploadFailed back to false when the upload progress resumes, the retryMessage persists.

jetpack/projects/plugins/jetpack/extensions/blocks/videopress/edit.native.js

Lines 172 to 174 in f1ae7fb

    
           if ( ! this.state.isUploadInProgress ) { 
        
           	this.setState( { isUploadInProgress: true, isUploadFailed: false } ); 
        
           }

Perhaps on line 375 we would need to upload the logic to isUploadFailed && ! isUploadInProgress to not display the failure retryMessage. (There may be other solutions.) I can help debug this further next week from the Upload Offline UX work if we're still experiencing it. Or, if it is only a soft blocker to this PR and we want to merge this work to reduce complexity, we can address it as a quick follow-up PR for next week.

derekblank · 2024-02-23T06:36:02Z

🟡 No transition on the upload progress bar
On Android, I also noticed that the upload progress bar goes from 0 to 100 with no transition. I tested other media blocks and I experienced the same behavior, so seems this is not related to this PR. I tested version 24.1 and this issue can't be reproduced so might be related to recent changes.

I was able to reproduce this for Android only on this PR's prototype build pr20181-f72c15e and the current trunk. When logging out the value of progress, the value remained stuck at 0.39179655723273754 and never progressed. As it seems to affect Android only, perhaps there is something in the Android host app code that is not updating the progress value (or sending it to the Editor).

I was not able to reproduce this in the 24.3-rc-1 beta. I created wordpress-mobile/gutenberg-mobile#6667 to investigate further. I agree with Carlos and don't consider this a PR blocker as it exists in trunk.

projects/plugins/jetpack/extensions/blocks/videopress/edit-common-settings.native.js

Feedback here: #35637 (comment)

fluiddot

Hey @SiobhyB, I tested the recent changes and confirmed the blocking issues I shared in #35637 (review) are now solved 🎊 . However, I noticed a new one (Empty state when using source URL of VideoPress video) that we might consider addressing in this PR. I'm hesitant about it because it might be a bit of an edge case, WDYT?

✅ Inconsistent UI when retrying a failed upload

Fixed in 1955fc1 !

🟡 Exceprt shows the VideoPress URL

It can still be reproduced as shared in #35637 (review), but it's not a blocker for this PR.

🟡 No transition on the upload progress bar

It can still be reproduced but will be addressed in a separate PR (#35637 (comment)).

❌ Empty state when using source URL of VideoPress video

By inserting the source of a VideoPress URL, I managed to create a Video block that contains the guid attribute but without the id. This is rendered properly on the web version, but not in native where it's displayed as empty.

Steps:

Copy the following URL: https://videos.files.wordpress.com/qDiVCMnq/0e2809330-1-2.mp4. This URL was obtained by getting the download URL from the VideoPress player.
Create a post in a Simple site.
Add a Video block.
Insert the copied URL.
Observe that the video can be played within the editor.
Save the post and open it again.
Observe that the post is marked as dirty. In this step, the video URL changed to the regular VideoPress URL.
Save the post and open it again.
Observe that the block is in the empty state.
Preview the post.
Observe that the video can be played.

Simulator.Screen.Recording.-.iPhone.15.-.2024-02-23.at.17.05.04.mp4

I tested the same behavior in version 24.2 and I can't reproduce it, so this seems a regression. If we decide that this will be covered in a separate PR or marked as low-priority due to being an edge case, I'll be happy to approve the PR.

SiobhyB · 2024-02-23T16:45:50Z

@fluiddot, thank you again for the thorough review! Regarding the outstanding issues:

Inconsistent UI when retrying a failed upload

I applied a fix in 1955fc1, but would like to investigate more why it's necessary to manually reset the states here, though it isn't in the Video block. @derekblank, I'd love to chat more next weeks given your insights from the offline uploads project. 🙇‍♀️

Excerpts shows the VideoPress URL

I'm currently working my way up the tree of PRs, and plan to address this one in the native PRs. Thanks for flagging!

To Be Addressed Separately

I'll plan to address the following issues separately (listed in order or priority):

Empty state when using source URL of VideoPress video
Plays inline setting not saving for Atomic sites w/o VideoPress. (Note, this setting is currently broken for all site types in production, so we would not be shipping a regression. I'm wary of making a change to a web file as part of this PR, so would prefer to create a separate PR for easier testing.)
Work on automated tests.
Revert [RNMobile] Video block: Fix logic for displaying empty state based on source presence WordPress/gutenberg#58015

I'll also create issues to keep track of the following, though may not personally tackle these myself in the short-term:

No transition on the upload progress bar
Add more settings to match web

Let me know if I'm missing anything here!

fluiddot

@fluiddot, thank you again for the thorough review! Regarding the outstanding issues:
[...]
Let me know if I'm missing anything here!

You perfectly covered all the issues in #35637 (comment), thanks @SiobhyB 🙇 ! As we considered addressing them in separate PRs, I'll proceed with approving the PR. LGTM 🎊 !

Regarding the issues, I'll be happy to help out in spare time between projects, so we could create associated GitHub issues and share the work. WDYT?

SiobhyB · 2024-02-23T17:03:39Z

Thank you @fluiddot! 🙌

Regarding the issues, I'll be happy to help out in spare time between projects, so we could create associated GitHub issues and share the work. WDYT?

That sounds great, thank you! I think I'll also defer the Excerpts shows the VideoPress URL issue to a separate PR. It'll be a small change, but I'm thinking it'd be nice to get this set of PRs merged so then we can iterate a bit quicker with further changes.

With that, I'll begin the merge domino shortly and create a mini project board to capture all of these issues. 🚀

SiobhyB · 2024-02-23T18:09:03Z

To follow up, I've created this GitHub board to organise the pending tasks to further refine this block. Thanks again for all the help merging the initial version! 🙇‍♀️

Siobhan added 5 commits February 12, 2024 14:55

feat: Enable usage of v5's editor component

3b4fe9b

docs: Add changelog entry

07a7b96

feat: Dupe Video block for basis of v5 support

ea3a0a9

With this commit, the main Video block files have been duplicated. They will be iterated on in subsequent commits and serve the basis of the changes we'll make to support VideoPress v5.

feat: Fetch VideoPress metadata in edit component

f5e9233

The Video block's is refactored to fetch VideoPress metadata, therefore enabling support for v5.

fix: Ensure plays inline toggle works as expected

79b062d

SiobhyB added the [Feature] VideoPress A feature to help you upload and insert videos on your site. label Feb 13, 2024

github-actions bot added [Block] VideoPress [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Feb 13, 2024

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 Feb 13, 2024

fix: Remove typo in settings component

6c8d50e

SiobhyB added the DO NOT MERGE don't merge it! label Feb 13, 2024

This was referenced Feb 13, 2024

Initial support for VideoPress v5 wordpress-mobile/gutenberg-mobile#6634

Merged

Initial support for VideoPress v5 wordpress-mobile/WordPress-Android#20181

Merged

refactor: Update logic for fetching video's URL

87db30a

SiobhyB mentioned this pull request Feb 13, 2024

Initial support for VideoPress v5 wordpress-mobile/WordPress-iOS#22602

Merged

13 tasks

feat: Ensure poster displays in thumbnail

a5d7df8

SiobhyB commented Feb 13, 2024

View reviewed changes

Siobhan added 4 commits February 14, 2024 14:14

fix: Ensure isUploadFailed state is always correct

9fecddc

feat: Show activity indicator if video's uploading

92904ee

refactor: Ensure fetchMetadata logic is correct

1aa07e5

refactor: Call poster directly, no redundant var

1b98f61

SiobhyB removed the DO NOT MERGE don't merge it! label Feb 14, 2024

SiobhyB requested review from fluiddot and derekblank February 14, 2024 16:52

Siobhan added 3 commits February 22, 2024 15:38

fix: Ensure failed state is reset when users retry

1955fc1

refactor: Tweaks to allow reusing translations

95a7277

The strings used in the block's settings have been tweaked to match the web, so that translations can be reused.

refactor: Align id definition with the video block

f1ae7fb

Addresses feedback here: #35637 (comment)

fluiddot reviewed Feb 23, 2024

View reviewed changes

projects/plugins/jetpack/extensions/blocks/videopress/edit-common-settings.native.js Outdated Show resolved Hide resolved

feat: Enable embed block convert after URL insert

e1de824

Feedback here: #35637 (comment)

fluiddot reviewed Feb 23, 2024

View reviewed changes

refactor: Fix missing period at end of sentence

e88d3d4

fluiddot approved these changes Feb 23, 2024

View reviewed changes

SiobhyB merged commit 68f87a4 into trunk Feb 23, 2024
53 of 54 checks passed

SiobhyB deleted the rnmobile/support-for-videopress-v5 branch February 23, 2024 17:10

github-actions bot removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Feb 23, 2024

github-actions bot added this to the jetpack/13.2 milestone Feb 23, 2024

This was referenced Feb 23, 2024

Video Block missing required metadata and using incorrect source in post_content after going Atomic when Gutenberg active wordpress-mobile/WordPress-Android#17751

Closed

Memory related crash after media upload wordpress-mobile/WordPress-iOS#20882

Closed

dcalhoun mentioned this pull request Feb 26, 2024

RNMobile: Detect and avoid invalid media URLs in VideoPress v5 block #35961

Merged

3 tasks

This was referenced Feb 27, 2024

RNMobile: Ensure playsInline attribute conforms when falling back to core Video block #35981

Merged

Release 1.114.0 wordpress-mobile/gutenberg-mobile#6687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Initial support for VideoPress v5 #35637

[RNMobile] Initial support for VideoPress v5 #35637

SiobhyB commented Feb 13, 2024 •

edited

Loading

github-actions bot commented Feb 13, 2024 •

edited

Loading

github-actions bot commented Feb 13, 2024

SiobhyB Feb 13, 2024 •

edited

Loading

SiobhyB Feb 13, 2024

SiobhyB Feb 13, 2024

fluiddot commented Feb 15, 2024 •

edited

Loading

SiobhyB commented Feb 15, 2024

derekblank commented Feb 23, 2024

derekblank commented Feb 23, 2024

fluiddot left a comment •

edited

Loading

SiobhyB commented Feb 23, 2024 •

edited

Loading

fluiddot left a comment

SiobhyB commented Feb 23, 2024

SiobhyB commented Feb 23, 2024

	export function getVideoUrlBasedOnPrivacy( guid: VideoGUID, isPrivate: boolean ) {
	if ( isPrivate ) {
	return `https://video.wordpress.com/v/${ guid }`;
	}

	return `https://videopress.com/v/${ guid }`;
	}

[RNMobile] Initial support for VideoPress v5 #35637

[RNMobile] Initial support for VideoPress v5 #35637

Conversation

SiobhyB commented Feb 13, 2024 • edited Loading

Related PRs

Proposed changes:

Other information:

Jetpack product discussion

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

Testing instructions:

Verify fix for videos not displaying thumbnails when previewed/published and not playing in the browser when uploaded via the app

Verify fix for plays inline setting not saving

Verify improvement when working between the web and the app

Further test for regressions

github-actions bot commented Feb 13, 2024 • edited Loading

github-actions bot commented Feb 13, 2024

SiobhyB Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

SiobhyB Feb 13, 2024

Choose a reason for hiding this comment

SiobhyB Feb 13, 2024

Choose a reason for hiding this comment

fluiddot commented Feb 15, 2024 • edited Loading

SiobhyB commented Feb 15, 2024

derekblank commented Feb 23, 2024

derekblank commented Feb 23, 2024

fluiddot left a comment • edited Loading

Choose a reason for hiding this comment

✅ Inconsistent UI when retrying a failed upload

🟡 Exceprt shows the VideoPress URL

🟡 No transition on the upload progress bar

❌ Empty state when using source URL of VideoPress video

SiobhyB commented Feb 23, 2024 • edited Loading

Inconsistent UI when retrying a failed upload

Excerpts shows the VideoPress URL

To Be Addressed Separately

fluiddot left a comment

Choose a reason for hiding this comment

SiobhyB commented Feb 23, 2024

SiobhyB commented Feb 23, 2024

SiobhyB commented Feb 13, 2024 •

edited

Loading

github-actions bot commented Feb 13, 2024 •

edited

Loading

SiobhyB Feb 13, 2024 •

edited

Loading

fluiddot commented Feb 15, 2024 •

edited

Loading

fluiddot left a comment •

edited

Loading

SiobhyB commented Feb 23, 2024 •

edited

Loading