Skip to content

Conversation

@fgrzeskowiak
Copy link
Collaborator

No description provided.


if (mode == 2 || mode == 3) { //stream rotation is enabled
SizeCalculator.updateMatrix(rotation, width, height, isPreview, isPortrait, MVPMatrix);
if(mode == 2 || mode == 3) {

Choose a reason for hiding this comment

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

Don't we want this logic here as well? Btw wrong formatting next to if

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the if completely

Choose a reason for hiding this comment

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

I guess we need those ifs, otherwise it may break logic for mode==0 and mode==1
Maybe you could leave previous logic as was and after updateMatrix call method like


if (!isPreview) {
   SizeCalculator.applyStreamFlip(isFlipHorizontal, isFrilVertical) {
       Matrix.setIdentityM(MVPMatrix, 0);
       Matrix.scaleM(MVPMatrix, 0, flipStreamHorizontal ? -1 : 1, flipStreamVertical ? -1 : 1, 1f);
    }
}


if (mode == 2 || mode == 3) { //stream rotation is enabled
SizeCalculator.updateMatrix(rotation, width, height, isPreview, isPortrait, MVPMatrix);
if (mode == 2 || mode == 3 || flipStreamHorizontal || flipStreamVertical) { //stream rotation is enabled

Choose a reason for hiding this comment

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

Do we need those flips in the if check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we need, because the whole flipping happens in updateMatrix method, which originally executes only when aspectRatioMode is 2 or 3. We're not changing those modes here, so additional condition is needed to execute this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The other option would be to remove this if completely and move it to the updateMatrix method itself.

public Camera1ApiManager(SurfaceTexture surfaceTexture, Context context, CameraSwitchCallback cameraSwitchCallback) {
this.surfaceTexture = surfaceTexture;
this.context = context;
this.cameraSwitchCallback = cameraSwitchCallback;

Choose a reason for hiding this comment

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

Do we need this in constructor? I guess it should be optional

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

private FaceDetectorCallback faceDetectorCallback;

public Camera1ApiManager(SurfaceView surfaceView, GetCameraData getCameraData) {
public Camera1ApiManager(SurfaceView surfaceView, GetCameraData getCameraData, CameraSwitchCallback cameraSwitchCallback) {

Choose a reason for hiding this comment

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

OpenGlViews works only for Camera2 api, so there is no need to add this callback here I guess?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed, it works for Camera1

Matrix.scaleM(MVPMatrix, 0, scale.x, scale.y, 1f);
if (!isPreview && !isPortrait) rotation += 90;
Matrix.rotateM(MVPMatrix, 0, rotation, 0f, 0f, -1f);
if (!isPreview && isCameraFront) {

Choose a reason for hiding this comment

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

I think that user should decide whether he wants to flip front or back camera

Choose a reason for hiding this comment

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

I think that isCameraFront param is not needed and user should decide whether to flip it.
So basically you don't need a callback for camera at all, or you could move it to Camera2ApiManager

so on theuser side it would be like

fun switchCamera {
    try {
       livestream.switchCamera()
   } finally {
       liveStream.glInterface.flipStreamVertical = livestream.isFrontCamera
   }
}

or

livstream.cameraChangeListener() { isFrontCamera ->
   liveStream.glInterface.flipStreamVertical = isFrontCamera
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

However, I'm not sure if putting the flip in finally block is a good idea. After all, it's executed after catch as well.

private int encoderWidth, encoderHeight;
private boolean loadAA = false;
private int streamRotation;
private int flipStreamMode = 4;

Choose a reason for hiding this comment

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

what is it for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can be removed.

this.context = context;
}

public OffScreenGlThread(Context context, boolean isFlipStreamHorizontal, boolean isFlipStreamVertical) {

Choose a reason for hiding this comment

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

why is it in constructor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I thought, that if for OpenGlView you can set isStreamFlipHorizontal and isStreamFlipVertical in xml attrubutes, the natural way for OffScreenGlThread, which is not in xml, would be to set them in constructor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But according to comment #2 (comment) I'll remove it.


@Override
public void setIsFlipStreamVertical(boolean isStreamFlipVertical) {

Choose a reason for hiding this comment

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

don't we want this feature for this class?


if (mode == 2 || mode == 3) { //stream rotation is enabled
SizeCalculator.updateMatrix(rotation, width, height, isPreview, isPortrait, MVPMatrix);
if(mode == 2 || mode == 3) {

Choose a reason for hiding this comment

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

I guess we need those ifs, otherwise it may break logic for mode==0 and mode==1
Maybe you could leave previous logic as was and after updateMatrix call method like


if (!isPreview) {
   SizeCalculator.applyStreamFlip(isFlipHorizontal, isFrilVertical) {
       Matrix.setIdentityM(MVPMatrix, 0);
       Matrix.scaleM(MVPMatrix, 0, flipStreamHorizontal ? -1 : 1, flipStreamVertical ? -1 : 1, 1f);
    }
}

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.

3 participants