Skip to content

Conversation

@ChiaoteNi
Copy link

@ChiaoteNi ChiaoteNi commented Jun 18, 2024

Descriptions:

  • To fix the following crash caused by a fetal error from GPUImage3
  • The exception occurs when the makeTexture function of the MTLDevice returns nil; it is associated with some memory issue, which we have fixed. However, it's still safer not to use fetal error in this case.

Solution:

  • Following the solution from CBVisualEffectBuiltIn, we skip the process when this case occurs
  • Force unwrap the value, similar to the original behavior using fetalError, for those that won't be used in PicCollage

Topics for discussion:

In CBVisualEffectBuiltIn, we use the input as the output directly when the makeTexture function return nil:

  1. Should we follow this behavior, then pass the inputTexture to the next filter by the newTextureAvailable?
    • The user may be confused due to there will be a result, but without any changes.
  2. Or just skip the process by stopping the invoking chain (current implementation in this PR)
    • It's more obvious to let the user know that there's a bug here, because there will be no image of the result.

@ChiaoteNi ChiaoteNi self-assigned this Jun 18, 2024
@ChiaoteNi ChiaoteNi requested a review from yyjim June 18, 2024 11:15
public init(size:Size) {
self.size = size
internalTexture = Texture(device:sharedMetalRenderingDevice.device, orientation:.portrait, width:Int(size.width), height:Int(size.height), timingStyle:.stillImage)
internalTexture = Texture(device:sharedMetalRenderingDevice.device, orientation:.portrait, width:Int(size.width), height:Int(size.height), timingStyle:.stillImage)!
Copy link
Member

Choose a reason for hiding this comment

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

question: Is it safe to use force unwrap here?

@ChiaoteNi ChiaoteNi changed the base branch from master to feat/faceMask-for-kirakira-effect June 19, 2024 01:34
@peteranny
Copy link

I vote option 2

@peteranny
Copy link

I vote option 2

Sorry please ignore it

@ChiaoteNi ChiaoteNi requested a review from yyjim June 19, 2024 03:34
@ChiaoteNi
Copy link
Author

Hi @yyjim, I've updated the code with the option 1, please help to review the PR again when you're available, thanks!

else {
assertionFailure("CommandBuffer or Texture creation failed")
removeTransientInputs()
textureInputSemaphore.signal()
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Yes we should, and I notice it shouldn't be invoke inside the defer
Let we add and remove them in the next commit, thanks!

Copy link
Member

@yyjim yyjim Jun 19, 2024

Choose a reason for hiding this comment

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

I'm not really sure if this correct or not. The L138 call also wait.

Copy link
Member

Choose a reason for hiding this comment

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

Calling it in defer seems reasonable to me since it calls wait at the beginning.

it only wants to run one at the time.

Copy link
Author

@ChiaoteNi ChiaoteNi Jun 19, 2024

Choose a reason for hiding this comment

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

That's weird...sorry I didn't notice this 🤔
I'll go back to check this later after I update the implementation for the PR of the face mask.

@ChiaoteNi ChiaoteNi requested a review from yyjim June 19, 2024 03:56
- Stop the process when it occurs in the newTextureAvailable
- Force unwrap the value for those who won't be used in PicCollage
@ChiaoteNi ChiaoteNi force-pushed the refactor/remove-fetalError-from-the-constructor-of-Texture branch from 646d38d to 870abf3 Compare June 19, 2024 05:03
@ChiaoteNi ChiaoteNi changed the base branch from feat/faceMask-for-kirakira-effect to master June 19, 2024 05:03
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