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

Reuse decoder instances on re-preparation + when seeking to a different period #2826

Closed
ojw28 opened this issue May 15, 2017 · 14 comments
Closed
Assignees

Comments

@ojw28
Copy link
Contributor

ojw28 commented May 15, 2017

ExoPlayer V2 re-uses decoder instances where possible as it plays through multiple periods of content (e.g. that have been concatenated using ConcatenatingMediaSource).

However the decoders are always released and re-instantiated if the user explicitly seeks to a period other than the one currently being played, even in cases where it's possible that the instances could be retained and re-used. We should look at optimizing this case.

@ojw28 ojw28 self-assigned this May 15, 2017
@ojw28 ojw28 changed the title Re-use decoder instances where possible when seeking to a different period. Re-use decoder instances when seeking to a different period May 15, 2017
@ojw28
Copy link
Contributor Author

ojw28 commented May 15, 2017

We should also look at re-using decoder instances when a new MediaSource is passed via prepare(), where it's possible to do so.

@zsmatyas
Copy link
Contributor

+1 😉

@ojw28 ojw28 changed the title Re-use decoder instances when seeking to a different period Re-use decoder instances on re-preparation + when seeking to a different period Feb 13, 2018
@BrainCrumbz
Copy link
Contributor

Is there any help we can provide in improving this aspect? Have you already thought about potential design changes, or already have an idea that we can try to implement? TA

@ojw28
Copy link
Contributor Author

ojw28 commented Sep 24, 2018

Work is in progress. There's a design proposal here. Feel free to post any comments you have. I don't think we need any help on the implementation side.

ojw28 added a commit that referenced this issue Sep 27, 2018
More specifically, if the parts of the Format that are used
for decoder configuration are unchanged.

Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=214791234
@ojw28 ojw28 changed the title Re-use decoder instances on re-preparation + when seeking to a different period Reuse decoder instances on re-preparation + when seeking to a different period Oct 3, 2018
ojw28 added a commit that referenced this issue Oct 15, 2018
- It's a no-op for now
- Renderers that want to support retaining resources will move
  some functionality from their disable() implementations into
  reset()
- ExoPlayerImplInternal will be updated to not always call reset()
  immediately after disable(), which is what will enable resources
  to actually be retained.

Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=215729783
ojw28 added a commit that referenced this issue Oct 15, 2018
For decoder reuse, we want disable() to flush the decoder. However,
if the flush needs to release the decoder for some reason, it seems
non-ideal to immediately re-initialize it. Re-initialization can
also throw ExoPlaybackException, which we don't want for disabling.

This change allows a variant of flush that wont re-initialize the
decoder if it has to be released, which will be used from disable().

Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=216834862
ojw28 added a commit that referenced this issue Oct 15, 2018
Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=216852679
ojw28 added a commit that referenced this issue Oct 18, 2018
Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=217189082
ojw28 pushed a commit that referenced this issue Oct 18, 2018
*** Reason for rollback ***

Broke photos (b/117818304)

*** Original change description ***

Add ExoPlayer.setForegroundMode

Issue: #2826

***

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=217356705
ojw28 added a commit that referenced this issue Oct 18, 2018
*** Reason for rollback ***

Photos regression is resolved by []

*** Original change description ***

Automated g4 rollback of changelist 217189082.

*** Reason for rollback ***

Broke photos (b/117818304)

*** Original change description ***

Add ExoPlayer.setForegroundMode

Issue: #2826

***

***

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=217773278
ojw28 added a commit that referenced this issue Oct 24, 2018
There are multiple subtle issues with the current implementation:

1. setOperatingRate can cause a codec initialization even if the
   renderer is disabled. This is not supposed to happen.
2. If the codec is released whilst the renderer is disabled, the
   renderer can instantiate a new codec using the old format when
   it's enabled again, only to immediately have to reconfigure or
   release it if the actual format to be played is different.
3. Codec reuse does not take into account renderer configuration.
   The specific case where this is problematic is if the video
   renderer is re-enabled with a different tunneling session id.
   The reused codec is then not configured correctly.

Also moved availableCodecInfos reset into releaseCodec for sanity.

Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=217924592
ojw28 added a commit that referenced this issue Oct 24, 2018
Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=218332277
ojw28 added a commit that referenced this issue Oct 24, 2018
Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=218346327
ojw28 added a commit that referenced this issue Oct 31, 2018
Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=218540967
ojw28 added a commit that referenced this issue Oct 31, 2018
This paves the way to cleanly fix the first two issues
listed in [] onDisabled will null inputFormat,
but nulling of codecFormat will remain tied to the codec
being released.

Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=219125458
ojw28 added a commit that referenced this issue Oct 31, 2018
Issue: #2826

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=219130576
ojw28 pushed a commit that referenced this issue Nov 1, 2018
*** Original change description ***

Re-enable codec re-use

Issue: #2826

***

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=219585084
ojw28 added a commit that referenced this issue Nov 6, 2018
*** Reason for rollback ***

Rolling forward again as [] should fix issue that prompted the rollback

*** Original change description ***

Automated g4 rollback of changelist 219130576.

*** Original change description ***

Re-enable codec re-use

Issue: #2826

***

***

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=220124362
@Romantic-LiXuefeng
Copy link

Romantic-LiXuefeng commented Nov 9, 2018

Work is in progress. There's a design proposal here. Feel free to post any comments you have. I don't think we need any help on the implementation side.

@ojw28 I read the linked design proposal. It's so great. Is there a similar public design documents? If have, please give me the links. I am reading the ExoPlayer source code.Thanks!

@ojw28
Copy link
Contributor Author

ojw28 commented Nov 9, 2018

Thanks for the feedback! There aren't, but I think it's a good idea for us to try and publish more of our design documents externally. This was a bit of a trial run for doing that.

@Romantic-LiXuefeng
Copy link

@ojw28 Reselect tracks manually when playing HLS stream ,and then decoder instances re-init. Whether reuse decoder instances at this situation is feasible?

@ojw28
Copy link
Contributor Author

ojw28 commented Dec 21, 2018

That doesn't happen when I try, except in the case where the re-selection is selecting a video stream bigger than anything that the decoder was previously configured to play (in this case release and re-initialization is necessary because the existing decoder wont have sufficiently large buffers).

@Romantic-LiXuefeng
Copy link

Romantic-LiXuefeng commented Dec 24, 2018

except in the case where the re-selection is selecting a video stream bigger than anything that the decoder was previously configured to play

Dose this mean re-selection from low bit rate video stream to high bit rate video stream?

@ojw28
Whether I switch from high bit rate to low bit rate or from low bit rate to high bit rate, The video decoder instances always needs release and re-initialization.

ExoPlayer version: 2.9.0
Test url :

{
        "name": "Apple 16x9 basic stream",
        "uri": "https://devstreaming-cdn.apple.com/videos/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8"
 },

@Romantic-LiXuefeng
Copy link

@ojw28 Did you already had the time to look into this? Is there anything I can do to support you?

@ojw28
Copy link
Contributor Author

ojw28 commented Jan 2, 2019

Decoder re-use is not part of the 2.9.0 release, or any release to date. It's in the dev-v2 branch only. Also note that this is still an open issue.

@ojw28
Copy link
Contributor Author

ojw28 commented Apr 22, 2019

Closing because this is supported in 2.10.0 (there is an alpha release available, and the final release is coming soon).

@ojw28 ojw28 closed this as completed Apr 22, 2019
@Romantic-LiXuefeng

This comment has been minimized.

@ojw28

This comment has been minimized.

@google google locked and limited conversation to collaborators Aug 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants