Fix indeterminate z-order of EditedMediaItemSequences #1055

AradiPatrik · 2024-01-31T03:11:35Z

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.

google-cla · 2024-01-31T03:11:39Z

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.

dway123 · 2024-01-31T19:16:49Z

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 · 2024-02-05T17:43:29Z

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.

libraries/effect/src/main/java/androidx/media3/effect/VideoCompositor.java

dway123

(some small comments at a glance :P )

libraries/effect/src/main/java/androidx/media3/effect/MultipleInputVideoGraph.java

libraries/transformer/src/main/java/androidx/media3/transformer/AudioSampleExporter.java

claincly

Look good overall!

claincly · 2024-02-23T15:37:07Z

libraries/effect/src/main/java/androidx/media3/effect/MultipleInputVideoGraph.java

  }

  @Override
  public VideoFrameProcessor getProcessor(int inputId) {
-    checkState(inputId < preProcessors.size());
+    checkState(preProcessors.indexOfKey(inputId) >= 0);


nit: Can use Util.contains(preProcessors, inputId) here.

claincly · 2024-02-23T15:39:49Z

libraries/common/src/main/java/androidx/media3/common/VideoGraph.java

@@ -75,14 +75,13 @@ interface Listener {
   *
   * <p>If the method throws, the caller must call {@link #release}.
   *
-   * @return The id of the registered input, which can be used to get the underlying {@link
-   *     VideoFrameProcessor} via {@link #getProcessor(int)}.
+   * @param sequenceIndex The sequence index of the input which can aid ordering of the inputs.


It seems it's easier if we require sequence indices start from 0, so please add something like "The index must start from zero"

claincly · 2024-02-23T15:40:35Z

libraries/effect/src/main/java/androidx/media3/effect/SingleInputVideoGraph.java

@@ -38,7 +38,7 @@
 @UnstableApi
 public abstract class SingleInputVideoGraph implements VideoGraph {

-  /** The ID {@link #registerInput()} returns. */
+  /** The ID {@link #registerInput(int)} returns. */


nit: The index of the only {@linkplain #registerInput(int) registered} input.

claincly · 2024-02-23T15:41:28Z

libraries/effect/src/main/java/androidx/media3/effect/VideoCompositor.java

@@ -48,8 +48,11 @@ interface Listener {
  /**
   * Registers a new input source, and returns a unique {@code inputId} corresponding to this
   * source, to be used in {@link #queueInputTexture}.
+   *
+   * @param sequenceIndex The sequence index of the input source, which is can be used to determine


Please reword: which is used to determine the order of the input sources

libraries/transformer/src/main/java/androidx/media3/transformer/AudioSampleExporter.java

claincly · 2024-02-23T15:44:14Z

libraries/transformer/src/main/java/androidx/media3/transformer/SampleExporter.java

@@ -64,10 +64,11 @@ public SampleExporter(Format firstInputFormat, MuxerWrapper muxerWrapper) {
   *
   * @param editedMediaItem The initial {@link EditedMediaItem} of the input.
   * @param format The initial {@link Format} of the input.
+   * @param sequenceIndex The sequence index of the input.


supernit: The index of the input sequence?

AradiPatrik · 2024-02-26T01:38:46Z

@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.

claincly · 2024-03-04T11:58:19Z

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!

AradiPatrik · 2024-03-22T07:23:16Z

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

claincly · 2024-03-22T11:47:04Z

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

yuriikonovalov · 2024-04-03T18:50:11Z

@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?

droid-girl · 2024-04-19T14:43:36Z

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

…enceIndex when registeringInput

droid-girl self-requested a review January 31, 2024 11:33

droid-girl self-assigned this Jan 31, 2024

droid-girl removed their request for review January 31, 2024 11:34

dway123 requested review from claincly and removed request for claincly January 31, 2024 19:14

dway123 requested a review from claincly February 5, 2024 17:39

AradiPatrik force-pushed the z-order-fix branch from 3c65e02 to 61b1c66 Compare February 6, 2024 03:18

droid-girl assigned claincly Feb 7, 2024

claincly force-pushed the z-order-fix branch from 61b1c66 to b9fda89 Compare February 7, 2024 21:08

claincly reviewed Feb 7, 2024

View reviewed changes

libraries/effect/src/main/java/androidx/media3/effect/VideoCompositor.java Outdated Show resolved Hide resolved

dway123 reviewed Feb 22, 2024

View reviewed changes

libraries/effect/src/main/java/androidx/media3/effect/MultipleInputVideoGraph.java Show resolved Hide resolved

libraries/transformer/src/main/java/androidx/media3/transformer/AudioSampleExporter.java Outdated Show resolved Hide resolved

droid-girl requested a review from claincly February 23, 2024 14:46

claincly requested changes Feb 23, 2024

View reviewed changes

claincly force-pushed the z-order-fix branch from a07377a to 9f48390 Compare March 4, 2024 11:58

claincly added the should merge label Mar 4, 2024

claincly force-pushed the z-order-fix branch 2 times, most recently from 8e76256 to 029ac25 Compare March 4, 2024 13:58

claincly force-pushed the z-order-fix branch 3 times, most recently from 15185e5 to aedf625 Compare May 3, 2024 14:33

claincly force-pushed the z-order-fix branch from aedf625 to 7577cdb Compare May 7, 2024 16:18

claincly force-pushed the z-order-fix branch from 7577cdb to 1a7a63c Compare May 7, 2024 23:42

AradiPatrik and others added 7 commits May 8, 2024 00:07

Fix indeterminate z-order of EditedMediaItemSequences by passing sequ…

177f1f3

…enceIndex when registeringInput

Use sparse array in MultipleInputVideoGraph, fix formatting

b909162

fix naming and formatting

84df55c

Format with google-java-format

3cccecf

implement top down approach for passing input source id

52adaf8

Refactor according to reviews.

51a9bcc

Format with google-java-format

0403e58

claincly force-pushed the z-order-fix branch from 1a7a63c to 3b7aa20 Compare May 8, 2024 00:07

Fix review comment

766ff44

claincly force-pushed the z-order-fix branch from 3b7aa20 to 766ff44 Compare May 8, 2024 00:11

copybara-service bot merged commit c5964d1 into androidx:main May 8, 2024
1 check passed

claincly mentioned this pull request May 29, 2024

If instances from different sequences are overlapping how are they mixed? Which one get's to be on top of the other? How to manipulate z-order? #1029

Closed

androidx locked and limited conversation to collaborators Jul 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix indeterminate z-order of EditedMediaItemSequences #1055

Fix indeterminate z-order of EditedMediaItemSequences #1055

AradiPatrik commented Jan 31, 2024

google-cla bot commented Jan 31, 2024

dway123 commented Jan 31, 2024

dway123 commented Feb 5, 2024

dway123 left a comment

claincly left a comment

claincly Feb 23, 2024

claincly Feb 23, 2024

claincly Feb 23, 2024

claincly Feb 23, 2024

claincly Feb 23, 2024

AradiPatrik commented Feb 26, 2024

claincly commented Mar 4, 2024

AradiPatrik commented Mar 22, 2024

claincly commented Mar 22, 2024

yuriikonovalov commented Apr 3, 2024 •

edited

Loading

droid-girl commented Apr 19, 2024

Fix indeterminate z-order of EditedMediaItemSequences #1055

Fix indeterminate z-order of EditedMediaItemSequences #1055

Conversation

AradiPatrik commented Jan 31, 2024

Root Cause:

Proposed Fix:

Implemented Solution:

Links

google-cla bot commented Jan 31, 2024

dway123 commented Jan 31, 2024

dway123 commented Feb 5, 2024

dway123 left a comment

Choose a reason for hiding this comment

claincly left a comment

Choose a reason for hiding this comment

claincly Feb 23, 2024

Choose a reason for hiding this comment

claincly Feb 23, 2024

Choose a reason for hiding this comment

claincly Feb 23, 2024

Choose a reason for hiding this comment

claincly Feb 23, 2024

Choose a reason for hiding this comment

claincly Feb 23, 2024

Choose a reason for hiding this comment

AradiPatrik commented Feb 26, 2024

claincly commented Mar 4, 2024

AradiPatrik commented Mar 22, 2024

claincly commented Mar 22, 2024

yuriikonovalov commented Apr 3, 2024 • edited Loading

droid-girl commented Apr 19, 2024

yuriikonovalov commented Apr 3, 2024 •

edited

Loading