media icon indicating copy to clipboard operation
media copied to clipboard

Fix indeterminate z-order of EditedMediaItemSequences

Open AradiPatrik opened this issue 1 year ago • 5 comments

Root Cause:

The indeterminate z-order issue arises due to the non-deterministic loading of assets. As each asset finishes loading, DefaultVideoCompositor.registerInput is invoked, linking the z-order to the unpredictable asset loading sequence. Consequently, the final z-order in the output is uncertain, as it's contingent on the load completion sequence of the assets.

Proposed Fix:

To resolve this, I've explored two options:

  • Option A: Modify TransformerInternal.java to aggregate all loaded assets first, then sequentially invoke registerInput in the intended order.
  • Option B: Introduce a sequenceIndex parameter to registerInput, decoupling the z-order from the invocation sequence and aligning it with this index instead.

Implemented Solution:

I opted for Option B due to its straightforward implementation. Post-fix, the z-order issue was no longer reproducible, indicating the effectiveness of this solution.

Links

I'm open to feedback and willing to assist further to ensure a robust resolution to this z-order inconsistency.

AradiPatrik avatar Jan 31 '24 03:01 AradiPatrik

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Jan 31 '24 03:01 google-cla[bot]

Thank you for the quick PR! I'll try to take a look in the next few days. Also, cc @claincly who is pretty familiar with these files too

dway123 avatar Jan 31 '24 19:01 dway123

I have less familiarity than @claincly here so I'll defer to him, but just noting that without as much context, I think the general option B selected in this PR sounds good to me (haven't looked to make sure the implementation style/architecture makes sense though). Option A would also require adding additional state and/or blocking, so I'd prefer B over A.

dway123 avatar Feb 05 '24 17:02 dway123

@claincly I implemented the changes you requested. Please take a look and resolve it if it's OK. Let me know if there's anything else we need to change before this PR can be merged.

AradiPatrik avatar Feb 26 '24 01:02 AradiPatrik

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

claincly avatar Mar 04 '24 11:03 claincly

@claincly Any updates on this? Can I assist in any way?

AradiPatrik avatar Mar 22 '24 07:03 AradiPatrik

Hey Aradi, we are discussing it internally as it's an API change, will keep you in the loop!

claincly avatar Mar 22 '24 11:03 claincly

@claincly could you provide some information (if it's possible) when it potentially can be merged and released so that this fix is available to use in production?

yuriikonovalov avatar Apr 03 '24 18:04 yuriikonovalov

The CL is still in review. We will try get back to you with a status update shortly

droid-girl avatar Apr 19 '24 14:04 droid-girl