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

Added an attribute to hide EMVideoView shutters #99

Closed
wants to merge 1 commit into from

Conversation

eygraber
Copy link
Contributor

The shutters were interfering with my implementation of CENTER_CROP. I toyed with the idea of using two layout files (one with shutters and one without) which would've had a slight performance increase (doesn't have to inflate the shutters and then remove them), but I figured it was simpler this way, and makes it easier to maintain exomedia_video_view_layout. Let me know if you want to go the two layout file route, and I'll put it in.

@brianwernick
Copy link
Owner

I wish I would have commented what issue I was fixing (or working around) with the shutters. It might be that I was fixing a background color issue with the SurfaceView but I'm not sure. My one thought is that since the TextureView resizes, we were seeing some oddities; if that's the case then removing the shutters to obtain a CenterCrop may not be ideal.

If the shutters are still needed, then we should probably be updating them as we calculate the change to the VideoTextureView (or the legacy VideoView).

Thoughts?

@eygraber
Copy link
Contributor Author

Maybe my use case would clear some things up. In my app, I want to playback video so that it takes up a rectangle that is the full width of the screen and 60% of the height. One of the requirements is that the video should always be full bleed, so that if it's ratio doesn't fit the rectangle's ratio it should crop from the center.

We play videos from two sources: one returns 480x480 videos and the other returns either 1080 or 720 videos. Since in most cases the 480 videos are much smaller than the rectangle, EMExoPlayer correctly scales it to fit. The larger videos get shutters on the side.

In order to implement center cropping, I borrowed ScaleManager and extended ExoVideoView to attach an ExoPlayer.Listener so I can apply the scale from onVideoSizeChanged (see setup() below). However, if the shutters are not removed, they still get calculated and displayed, so even though the video has scaled correctly, there are still shutters on the side and the video is no longer "full bleed."

An alternative that I tested before writing this PR was to extend EMVideoView and override updateVideoShutters to do nothing, and calculateVerticalShutterSize and calculateSideShutterSize to return 0, which worked. I felt that was a little hacky though.

OK now after writing that monolith, I re-read your comment. To answer that, I'm not relying on just removing the shutters to obtain center crop; I'm also replacing the VideoViewApi with the class below.

ScalableVideoView

public final class ScalableVideoView extends ExoVideoView {
  private ScalableType scalableType = ScalableType.NONE;

  public ScalableVideoView(Context context) {
    super(context);
    setup(null);
  }

  public ScalableVideoView(Context context, AttributeSet attrs) {
    super(context, attrs);
    setup(attrs);
  }

  public ScalableVideoView(Context context, AttributeSet attrs, int defStyleAttr) {
    super(context, attrs, defStyleAttr);
    setup(attrs);
  }

  @TargetApi(Build.VERSION_CODES.LOLLIPOP)
  public ScalableVideoView(Context context, AttributeSet attrs, int defStyleAttr, int defStyleRes) {
    super(context, attrs, defStyleAttr, defStyleRes);
    setup(attrs);
  }

  private void setup(@Nullable AttributeSet attrs) {
    if(attrs != null) {
      TypedArray a = getContext().obtainStyledAttributes(attrs, com.yqritc.scalablevideoview.R.styleable.scaleStyle, 0, 0);
      if (a == null) {
        return;
      }

      int scaleType = a.getInt(com.yqritc.scalablevideoview.R.styleable.scaleStyle_scalableType, ScalableType.NONE.ordinal());
      a.recycle();
      scalableType = ScalableType.values()[scaleType];
    }

    emExoPlayer.addListener(new ExoPlayerListener() {
      @Override
      public void onVideoSizeChanged(int width, int height, int unAppliedRotationDegrees, float pixelWidthHeightRatio) {
        scaleVideoSize(width, height);
      }

      @Override public void onStateChanged(boolean playWhenReady, int playbackState) {}
      @Override public void onError(EMExoPlayer emExoPlayer, Exception e) {}
    });
  }

  @Override
  public void updateAspectRatio(float aspectRatio) {
    // do nothing
  }

  private void scaleVideoSize(int videoWidth, int videoHeight) {
    if (videoWidth == 0 || videoHeight == 0) {
      return;
    }

    Size viewSize = new Size(getWidth(), getHeight());
    Size videoSize = new Size(videoWidth, videoHeight);
    ScaleManager scaleManager = new ScaleManager(viewSize, videoSize);
    Matrix matrix = scaleManager.getScaleMatrix(scalableType);
    if (matrix != null) {
      setTransform(matrix);
    }
  }
}

@brianwernick
Copy link
Owner

OK. So I was thinking to natively support different scale modes (including CENTER_CROP) a class similar to the ScaleManager could be built in so that the shutters pay attention to the video size after the scaling is applied. This would allow us to solve both #1 and #80 (and possibly #78)

@eygraber
Copy link
Contributor Author

I think that's a much better long term solution. I was using ScaleManager as a monkey patch. Since that library does a good job, it might be worth it to reuse it either as a fork with the unnecessary functionality removed, or getting permission to integrate just the source that is needed here.

@brianwernick
Copy link
Owner

With that in mind, is there any reason to keep this PR open? (I don't think so, but I may be missing something)

@eygraber
Copy link
Contributor Author

It's not needed if the shutters could be influenced by the scale type (e.g. if the scale type is CENTER_CROP then the shutters would not be shown, etc...).

@brianwernick
Copy link
Owner

Alright, I'll close the PR then.

@eygraber eygraber deleted the hide_shutters branch March 9, 2016 00:19
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.

2 participants