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

Fixing paused video doesn't react on changing device orientation #199

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kareljuricka
Copy link
Contributor

@kareljuricka kareljuricka commented Nov 27, 2020

Reacting on #194 I found that problem is in condiiton:
frame == prev_frame_
in apple_video_renderer.cc. It returns nullptr in this case and renderLoop inside ShakaPlayerView.mm doesn't pass:
if (CGImageRef image = _player.videoRenderer->Render(nullptr, &aspect_ratio)) {..
condition

So solution would be remove frame == prev_frame_, but this could brings unnecessary overhead to rendering loop.
So I limit this condition only for case, that video is paused. To make it possible, I have to make player_ property protected, to be inheritable from parent class. Also I had to move mutex_ up to player_, due to some compilations errors.
This works and bug is fixed.

I could imagine there should be better solution, directly in file ShakaPlayerView.mm - watching screen orientation change and only forcing recalculation in that case. Additionaly storing some informations from last render and use them for paused state. But I don't want to make huge updates in original source code.

Fixes #194

@TheModMaker
Copy link
Collaborator

This will have some major performance impact. When we're paused, users would expect the app to be doing (almost) nothing. This would require us to regenerate the same image 30+ times a second. This would be better done in ShakaPlayerView. The orientation and bounds have no effect on this generated image, so you should only have to change the contentsRect and frame fields of the image based on the new bounds. You basically just need to use this code in an event listener:

https://github.com/google/shaka-player-embedded/blob/9fc13728fa004d5928fbe72955ca3ebbae75ed8c/shaka/src/public/ShakaPlayerView.mm#L172-L188

@kareljuricka
Copy link
Contributor Author

I know it's overhead, but it was the simplest solution.

I tried to think your advice but:

shaka::ShakaRect<uint32_t> image_bounds = {0, 0, CGImageGetWidth(image), 
                                            CGImageGetHeight(image)}; 

returns wrong image size (image size from previous orientation) after resize and I have to call

CGImageRef image = _player.videoRenderer->Render(nullptr, &aspect_ratio)

to get correct new ones.

So how can I do it, without calling Render() method. I understand I can't call render method because frame == prev_frame_ returns nill as result.

@TheModMaker
Copy link
Collaborator

The base image is always the same, it is based on the image data without any resizing. We use contentsRect and bounds to resize the image to fit based on the orientation. We want the original image size for image_bounds.

@kareljuricka
Copy link
Contributor Author

kareljuricka commented Nov 30, 2020

Sorry, you're right. Image dimensions are really same, I was wrong.

I create new commit, where I reverse all previous patches and has working solution as you advice.
Two notes:

  1. I had to make image or image_bounds as class property to access it in orientationChanged method.. I deciced to image property. I didn't find out way how to access original image from _imageLayer.contents. Maybe (I hope) there is better way?

  2. I has to change self.bounds to self.superview.bounds because self.bounds returned wrong values.
    I made same change also in renderLoop. Then I could remove my workaround in swift project inside my ViewController:
    self.playerView.frame = self.view.bounds; which was used to fix problem previously (calling on orientation change), but worked only when player was playing.

shaka/src/public/ShakaPlayerView.mm Outdated Show resolved Hide resolved
shaka/src/public/ShakaPlayerView.mm Outdated Show resolved Hide resolved
shaka/src/public/ShakaPlayerView.mm Outdated Show resolved Hide resolved
shaka/src/public/ShakaPlayerView.mm Outdated Show resolved Hide resolved
shaka/src/public/ShakaPlayerView.mm Outdated Show resolved Hide resolved
@kareljuricka
Copy link
Contributor Author

kareljuricka commented Nov 30, 2020

I updated all points, except superview workaround. I will try to figure out why view.bounds are wrong.

UPDATE:
I don't think there is problem with calling it earlier, because in renderLoop, after orientation change, self.bounds results in previous dimensions and do not change to new one. All future rounds of renderLoop then returns wrong bounds.
So problem is probably with self.bounds, which aren't implicit recalculated on device orientation change...

@kareljuricka
Copy link
Contributor Author

kareljuricka commented Dec 1, 2020

Ok, I found better solution:
In my own ViewController, in which I inserting ShakaPlayerView, I moved

self.playerView.frame = self.view.bounds;

from viewWillAppear

inside another override function:

override func viewDidLayoutSubviews() {
  if (self.playerView != nil) {
     self.playerView.frame = self.view.bounds;
  }
}

with this solution, I can use ShakaPlayer bounds instead of super view bounds, because they are updated from parent.

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

Successfully merging this pull request may close these issues.

Paused video doesn't react on changing device orientation
2 participants