Skip to content
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

Add WrappingMediaSource #10446

Closed
wants to merge 39 commits into from
Closed

Conversation

stoyicker
Copy link

This is a CompositeMediaSource with a slightly simpler API. I thought it was a bit more readable than the current usage of CompositeMediaSource to signify a single-source one.

dway123 and others added 30 commits July 7, 2022 16:39
This is to be consistent with what cast `QueueMediaItem` is doing. If a contentId is
not available the contentUrl is used as the ID.

#minor-release

PiperOrigin-RevId: 459133323
This profile is declared as supported although it isn't.

Issue: google#10345
Issue: google#3537

#minor-release

PiperOrigin-RevId: 459205512
HDR editing is not supported under API 31

PiperOrigin-RevId: 459211106
PiperOrigin-RevId: 459215618
The SDR constant also specified a color space and range, in addition to
C.COLOR_TRANSFER_SDR. However, it turns out that SDR videos may use different color
space and range values, so following prior ExoPlayer conventions to have `null`
mean "generic SDR" is preferable here.

PiperOrigin-RevId: 459296746
We wait until a previous AudioTrack has been released before
creating a new one. This is currently done with a thread
block operation, which may cause ANRs in the extreme case
when someone attempts to release the player while this is
still blocked.

The problem can be avoided by just returning false from
DefaultAudioSink.handleBuffer to try again until the previous
AudioTrack is released.

Reproduction steps to force the issue:
1. Add Thread.sleep(10000); to the AudioTrack release thread.
2. Add this to the demo app:
    private int positionMs = 0;

    Handler handler = new Handler();
    handler.post(new Runnable() {
      @OverRide
      public void run() {
        player.seekTo(positionMs++);
        if (positionMs == 10) {
          player.release();
        } else {
          handler.postDelayed(this, 1000);
        }
      }
3. Observe Player release timeout exception.

These steps can't be easily captured in a unit test as we can't
artifically delay the AudioTrack release from the test.

Issue: google#10057
PiperOrigin-RevId: 459468912
PiperOrigin-RevId: 459485334
Some other minor nits and adjustments to the API logic.

PiperOrigin-RevId: 459490431
Also added the test to `MP4PlaybackTest`.

PiperOrigin-RevId: 459492188
#minor-release

Issue: google#10420
PiperOrigin-RevId: 460223064
- Added setter to disable this feature.
- Added accompanying tests.
- Plan to run tests on the same set of settings on H265.

PiperOrigin-RevId: 460238673
This saves an intermediate texture copy step for use-cases
where matrix transformations are the first or only effects
in the chain.

PiperOrigin-RevId: 460239403
The media item needs to be assigned to `Window.mediaItem` in `CastTimeline.setWindow`. For this the `MediaItem` needs to be available in the timeline.

When a `MediaItem` is passed to the `set/addMediaItems` method, we can't yet know the Cast `MediaQueueItem.itemId` that is generated on the device and arrives with an async update of the `RemoteMediaClient` state. Hence in the `CastTimelineTracker`, we need to store the `MediaItem` by Casts's `MediaItem.contentId`. When we then receive the updated queue, we look the media item up by the content ID to augment the `ItemData` that is available in the `CastTimeline`.

Issue: androidx/media#25
Issue: google#8212

#minor-release

PiperOrigin-RevId: 460325235
This extension is needed for editing HDR input with OpenGL, as the
ExternalTextureProcessor samples raw YUV values from the
external texture for HDR and converts them to RGB itself rather than
relying on the OpenGL driver to do this automatically as for SDR.

PiperOrigin-RevId: 460424154
Pass the color info and HDR static metadata when configuring the decoder
using MediaFormatUtil.maybeSetColorInfo.

PiperOrigin-RevId: 460424985
I don't think it's useful to keep these in numerical order, it makes
more sense to keep them grouped into a 'logical' ordering.

#minor-release

PiperOrigin-RevId: 460453464
Some calls to handleBuffer return false while a previous
flush is still handled in the background.

Fix this by either asserting the method returns true if
we don't expect any delay, or calling it repeatedly until
it returns true (within a timeout).

PiperOrigin-RevId: 460474419
`HevcConfig.parse` misreads reserved bit to determine NAL unit type. This is currently meant to be always set to 0, but could be given some kind of meaning in a future revision.

Issue: google#10366
PiperOrigin-RevId: 460487613
PiperOrigin-RevId: 460500666
Leaving the media item that has been passed in unchanged, ensures that the
media item in the timeline is equal to the media item that the user has
passed into the player. The value of the tag is the uid of the window,
meaning this is redundant information.

#minor-release

PiperOrigin-RevId: 460542246
Note: This was already reviewed in <unknown commit>. This doesn't mean we cannot apply further changes though.
PiperOrigin-RevId: 460542835
PiperOrigin-RevId: 460662425
#cleanup
#minor-release

PiperOrigin-RevId: 460688226
Samrobbo and others added 7 commits July 13, 2022 17:46
Add test that verifies SSIM with API enabled.

#minor-release

PiperOrigin-RevId: 460692420
We currently start a simple Thread to release AudioTracks
asynchronously. If many AudioTracks are released at the same
time, this may lead to OOM situations because we attempt to
create multiple new threads.

This can be improved by using a shared SingleThreadExecutor.
In the simple case of one simmultaneous release, it's exactly
the same behavior as before: create a thread and release it
as soon as it's done. For multiple simultanous releases we
get the advantage of sharing a single thread to avoid creating
more than one at the same time.

Issue: google#10057
PiperOrigin-RevId: 460698942
The call doesn't currently reset the already loaded suppliers and
factories. Also fix the supplier loading code to use a local copy
of the current dataSourceFactory to avoid leaking an updated
instance to a later invocation.

Issue: androidx/media#116

#minor-release

PiperOrigin-RevId: 460721541
Also remove VideoEncoderSettings.colorProfile as there are no
concrete use cases for customizing this and it clashes with picking
the color format automatically based on SDR vs. HDR.

PiperOrigin-RevId: 460746987
These are providing more variety and complexity.

All files are okay to be public.

PiperOrigin-RevId: 460935247
#minor-release

PiperOrigin-RevId: 461162552
@tonihei tonihei self-assigned this Jul 18, 2022
@@ -0,0 +1,124 @@
/*
* Copyright (C) 2018 The Android Open Source Project
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 2022 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

/**
* Single-child {@link CompositeMediaSource}.
*/
public abstract class SimpleMediaSource extends CompositeMediaSource<Void> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be caller to call this class WrappingMediaSource because "simple" doesn't really tell anyone what it's for.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* @param timeline The timeline of the source.
*/
protected abstract void onChildSourceInfoRefreshed(
MediaSource mediaSource, Timeline timeline
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of this class knows the wrapped inner mediaSource, so no need to pass it back here I think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I agree 100% - it of course knew when it called prepareChildSource, but for it to know when this is called it'd have to have kept a reference to it somehow. And that is possible, but so is that a regular CompositeMediaSource inheritor keeps a mapping between ids and mediasources itself, so that it could retrieve the child just by using the id parameter on CompositeMediaSource.onChildSourceInfoRefreshed, and therefore applying this same argument then the MediaSource parameter wouldn't be needed there either. Without wanting to create a discussion on whether that argument should be removed in CompositeMediaSource.onChildSourceInfoRefreshed, I think for the sake of consistency with CompositeMediaSource the argument is better kept here too. Am I making sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does make sense generally, but I think all implementations of this WrappingMediaSource will need to have a local reference to the wrapped child media source in any case. For example, the implementation couldn't create MediaPeriod from createPeriod if it doesn't have a reference to the child source.

I think for the sake of consistency with CompositeMediaSource

We are already changing the method signature by removing the useless argument, so there is no consistency to start with?

}

/**
* @deprecated - Use {@link #getWindowIndexForChildWindowIndex(int)}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method isn't really deprecated and you can just remove the comment entirely. Same for the other cases below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not deprecate it? Would receivers of this class really want a method that takes an id?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that our libraries never use @deprecated for cases where the method itself is redirected. There also shouldn't be any caller besides CompositeMediaSource itself, so it seems easier to the deprecated tag.

@tonihei
Copy link
Collaborator

tonihei commented Jul 18, 2022

Thanks for the contribution!

For reference: I believe this is solving #7279.

Is there any chance you can rebase this change on the main branch of the Media3 repo? This would make the merging process a lot easier for us.

@stoyicker stoyicker changed the title Add SimpleMediaSource Add WrappingMediaSource Jul 18, 2022
@stoyicker
Copy link
Author

I've rebased on top of 9271572 from media3, feel free to change the PR base for a simpler diff if you have a more fitting one

@tonihei
Copy link
Collaborator

tonihei commented Jul 19, 2022

I've rebased on top of 9271572 from media3

Sorry, I think my question was a bit unclear. I actually meant to file a new PR against the main branch of the Media3 project . The files look exactly the same, but have different package names.

@stoyicker
Copy link
Author

androidx/media#123

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet