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

fix video play error, on android. #1360

Closed
wants to merge 2 commits into from
Closed

fix video play error, on android. #1360

wants to merge 2 commits into from

Conversation

xianyinchen
Copy link
Contributor

@xianyinchen xianyinchen commented Jul 4, 2018

安卓下面的媒体播放器依赖于Surface渲染,如果Surface没有初始化,会导致媒体播放器无法正常工作。Surface的初始化,需要保证Surface的显示区域可见且大小有效。目前的代码逻辑会出现Surface无法正常工作的情况,上述2种情况都可能发生,详见代码注释。顺便修复了,切换后台后,播放状态的还原。

@jareguo jareguo mentioned this pull request Jul 4, 2018
@minggo
Copy link
Contributor

minggo commented Jul 5, 2018

可能得让对这块代码比较熟悉的人来 review。不过从 PR 的描述来看我仍然不知道解决的是什么问题。PR 的提交可以参考 :cocos2d/cocos2d-x#18733 。 虽然不要求描述得那么详细,但是知道得描述一下问题是什么,解决的思路是什么,如何重现等。

@@ -93,6 +93,7 @@
protected int mFullScreenHeight = 0;

private int mViewTag = 0;
private int mViewVisible = INVISIBLE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

新增一个状态位,用于保证Surface还没初始化之前,Surface的区域可见

@@ -105,7 +106,7 @@ public Cocos2dxVideoView(Cocos2dxActivity activity,int tag) {
@Override
protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
if (mVideoWidth == 0 || mVideoHeight == 0) {
setMeasuredDimension(mViewWidth, mViewHeight);
super.onMeasure(widthMeasureSpec, heightMeasureSpec);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

该处理会导致Surface的可是区域大小变为0,导致Surface无法初始化

Copy link
Contributor

Choose a reason for hiding this comment

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

为什么需要 videoWidth 和 visibleWidth?在什么情况下二者会不一致?我的问题和修改没关系,但是想了解一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

视频的实际播放大小,需要视频加载后才知道,设置URL的时候,默认给予0的区域大小。没有具体跟踪为什么visibleWidth和videoWidth为什么不一致。我改了当前位置的代码,是为了避免任何情况下导致的区域为0的情况。

Copy link
Contributor

Choose a reason for hiding this comment

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

如果说控件初始化必须保证大小不为0,那么是否应该修改 mVideoWidth 的默认值呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我认为在设置url的时候,就不该去修改mVideoWidth的值,我不确定原来设置该大小的目的,但是我在这个位置的代码需要需要保留,我们必须保证任何情况下,Surface都可以正常初始化,不然媒体播放器的很多问题,无法通过表现获得。

@@ -170,11 +171,13 @@ public int resolveAdjustedSize(int desiredSize, int measureSpec) {

@Override
public void setVisibility(int visibility) {
if (visibility == INVISIBLE) {
if(getCurrentPosition() > 0 && mSeekWhenPrepared == 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

记录播放位置的代码移到后台切换和暂停的时候触发,只有需要我们才去做,这样理解比较直观。

@@ -557,6 +560,7 @@ public void surfaceChanged(SurfaceHolder holder, int format,
public void surfaceCreated(SurfaceHolder holder)
{
mSurfaceHolder = holder;
setVisibility(mViewVisible);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

由于之前我们可能为了等待Surface初始化,强制了设置了是否可见,这里需要做下还原真正的状态。

@@ -578,16 +582,18 @@ public void surfaceDestroyed(SurfaceHolder holder)
*/
private void release(boolean cleartargetstate) {
if (mMediaPlayer != null) {
if (mTargetState == STATE_PLAYING) {
mSeekWhenPrepared = mMediaPlayer.getCurrentPosition();
mTargetState = STATE_PLAYING;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

当应用进入后台后,Surface会变得无效,我们这时候必须释放设备,如果视频还在播放中,则记录下当前位置,并保留原有状态,以便恢复之后还原状态。

Copy link
Contributor

Choose a reason for hiding this comment

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

mTargetState = STATE_PLAYING; 就没必要了吧,本来就是这个状态。

Copy link
Contributor

Choose a reason for hiding this comment

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

cleartargetstate 还有参数表明是否清除状态呢。这样改的话,这个参数就没用了,改变了原来的逻辑。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我这边是多写了一行代码,我是想表达这里的用途。修改逻辑之后,设备被释放的情况只有2种,一种是主动销毁播放组件,另一种是切换到后台,在这两种情况下, mTargetState = STATE_IDLE 的设置并没有实际意义,我是这么认为的。

Copy link
Contributor

Choose a reason for hiding this comment

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

如果是主动销毁播放组件,把状态设置为 playing 不是也很奇怪吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

在任何情况下,mTargetState的状态可以视为我们的引擎组件的逻辑状态,而非设备的真正运行状态,设备的丢失不意味我们的UI组件的播放状态被重置,如果有需要重置,说明使用者对UI组件做了逻辑处理,如果是销毁UI对象,mTargetState的状态就没有意义。这是我的理解,我认为 mTargetState 比较注重的是UI组件实际情况的保留与还原。

Copy link
Contributor

Choose a reason for hiding this comment

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

嗯,虽然是逻辑状态,但是它是 TARGET_STATE,也就是下一步想要的状态。

}

mTargetState = STATE_PLAYING;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

将mTargetState的标志移到下面来,以便媒体播放器初始化后,可以根据状态正常播放。因为媒体播放器的播放是异步的,如果用户加载视频并立即调用播放,这时由于播放器还没初始化,isInPlaybackState状态是false,导致播放状态是无法被正常设置,当播放器初始化,视频却不会播放。

public void pause() {
if (isInPlaybackState()) {
if (mMediaPlayer.isPlaying()) {
mSeekWhenPrepared = mMediaPlayer.getCurrentPosition();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

记录暂停时的播放位置,用于继续播放。

@cocos cocos deleted a comment from minggo Jul 5, 2018
@cocos cocos deleted a comment from minggo Jul 5, 2018
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mSurfaceHolder为空,代表者Surface还没初始化,这时候我们强制可视区域为可见,迫使Surface可以初始化成功。我放在这里吧。

Copy link
Contributor

Choose a reason for hiding this comment

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

强制设置为可见,后面再恢复成应有的状态。这个逻辑有点诡异。是否能初始化成功应该是系统控件自己要处理的逻辑。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是测试得出的,只要可见区域无效,Surface就不会初始化,这就是系统对视窗的处理逻辑。

Copy link
Contributor

Choose a reason for hiding this comment

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

我的意思是可能是初始化的逻辑或者顺序错了,不应该强制设置为可见让初始化成功。

@minggo
Copy link
Contributor

minggo commented Jul 5, 2018

我这边 review 的结果是:在没有对原本的设计比较清楚的情况下修改可能可以解决某些问题,但是可能会带来更多的问题。修改中有些地方比较 hack,从逻辑上不太说得通。

另外对 video 的设计也有几个问题:

  • 为什么会有 video width/heightvisible width/height
  • 为什么需要有 full screen width/height,既然全屏了,怎么还有大小一说,而且还是可以设置的

@xianyinchen
Copy link
Contributor Author

这次的修改在example-case上没有测试问题。对于以上的疑问,可能需要内部讨论一下,毕竟这个涉及到整个模块的设计,个人原因,没法用文本做一个比较完整性的阐述。

@minggo
Copy link
Contributor

minggo commented Jul 5, 2018

通过讨论觉得 video 的问题还是需要有专人维护去解决。@xianyingchen 目前没有资源做这个事情,所以等 @drelaptop 后面有空了来看一下 video 的 issue。

@minggo minggo closed this Jul 5, 2018
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.

None yet

2 participants