-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix indeterminate z-order of EditedMediaItemSequences #1055
Conversation
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. |
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 |
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. |
3c65e02
to
61b1c66
Compare
libraries/effect/src/main/java/androidx/media3/effect/VideoCompositor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(some small comments at a glance :P )
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good overall!
} | ||
|
||
@Override | ||
public VideoFrameProcessor getProcessor(int inputId) { | ||
checkState(inputId < preProcessors.size()); | ||
checkState(preProcessors.indexOfKey(inputId) >= 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can use Util.contains(preProcessors, inputId)
here.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems it's easier if we require sequence indices start from 0, so please add something like "The index must start from zero"
@@ -38,7 +38,7 @@ | |||
@UnstableApi | |||
public abstract class SingleInputVideoGraph implements VideoGraph { | |||
|
|||
/** The ID {@link #registerInput()} returns. */ | |||
/** The ID {@link #registerInput(int)} returns. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The index of the only {@linkplain #registerInput(int) registered} input.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reword: which is used to determine the order of the input sources
libraries/transformer/src/main/java/androidx/media3/transformer/AudioSampleExporter.java
Outdated
Show resolved
Hide resolved
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supernit: The index of the input sequence
?
@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. |
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! |
8e76256
to
029ac25
Compare
@claincly Any updates on this? Can I assist in any way? |
Hey Aradi, we are discussing it internally as it's an API change, will keep you in the loop! |
@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? |
The CL is still in review. We will try get back to you with a status update shortly |
15185e5
to
aedf625
Compare
…enceIndex when registeringInput
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:
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.