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.

Sign up for GitHub

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

[ads] Fix NTT video background may be blank after changing NTP media type setting #25285

Merged
merged 1 commit into from
Aug 23, 2024
Merged

[ads] Fix NTT video background may be blank after changing NTP media type setting #25285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions ...ces/Brave/Frontend/Browser/New Tab Page/Backgrounds/NewTabPageBackgroundButtonsView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class NewTabPageBackgroundButtonsView: UIView, PreferencesObserver {
private let playButton = PlayButton().then {
$0.isHidden = true
}
private var shouldShowPlayButton = false
private var isVideoBackground = false
private var playButtonGestureRecognizer: UITapGestureRecognizer?

/// The parent safe area insets (since UICollectionView doesn't feed down
Expand Down Expand Up @@ -175,6 +175,11 @@ class NewTabPageBackgroundButtonsView: UIView, PreferencesObserver {
tappedBackgroundDuringAutoplay?()
}

func resetVideoBackgroundButtons() {
isVideoBackground = false
updatePlayButtonVisibility()
}

func videoAutoplayStarted() {
let tapGesture = UITapGestureRecognizer(
target: self,
Expand All @@ -185,7 +190,7 @@ class NewTabPageBackgroundButtonsView: UIView, PreferencesObserver {
}

func videoAutoplayFinished() {
shouldShowPlayButton = true
isVideoBackground = true
updatePlayButtonVisibility()

if let playButtonGestureRecognizer = playButtonGestureRecognizer {
Expand All @@ -201,7 +206,7 @@ class NewTabPageBackgroundButtonsView: UIView, PreferencesObserver {
if isLandscape && UIDevice.isPhone {
playButton.isHidden = true
} else {
playButton.isHidden = !shouldShowPlayButton
playButton.isHidden = !isVideoBackground
}
}

Expand Down
5 changes: 5 additions & 0 deletions ...os/Sources/Brave/Frontend/Browser/New Tab Page/Backgrounds/NewTabPageBackgroundView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ class NewTabPageBackgroundView: UIView {
}
}

func resetPlayerLayer() {
playerLayer.player = nil
playerLayer.removeFromSuperlayer()
}

override init(frame: CGRect) {
super.init(frame: frame)

Expand Down
1 change: 0 additions & 1 deletion ...e-ios/Sources/Brave/Frontend/Browser/New Tab Page/Backgrounds/NewTabPageVideoPlayer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ class NewTabPageVideoPlayer {

init(_ backgroundVideoPath: URL) {
self.backgroundVideoPath = backgroundVideoPath
createPlayer()
}

deinit {
Expand Down
32 changes: 22 additions & 10 deletions ios/brave-ios/Sources/Brave/Frontend/Browser/New Tab Page/NewTabPageViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,18 @@ class NewTabPageViewController: UIViewController {
}

background.changed = { [weak self] in
self?.setupBackgroundImage()
guard let self else { return }
setupBackgroundImage()

let isTabVisible = viewIfLoaded?.window != nil
setupBackgroundVideoIfNeeded(shouldCreatePlayer: isTabVisible)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is a proper place to call setupBackgroundVideoIfNeeded after background is changed (which happens when user changes New Tab Page setting)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We also need to create AVPlayer object only if current tab is visible to avoid memory/processor consumption.
AVPlayer object will be created during willMove(...) when user switches to tab.

// Load the video asset here, as viewDidAppear is not called when the view
// is already visible
if isTabVisible {
videoPlayer?.loadAndAutoplayVideoAssetIfNeeded(
shouldAutoplay: false
)
}
}

Preferences.BraveNews.isEnabled.observe(from: self)
Expand Down Expand Up @@ -330,7 +341,7 @@ class NewTabPageViewController: UIViewController {
}

setupBackgroundImage()
setupBackgroundVideoIfNeeded()
setupBackgroundVideoIfNeeded(shouldCreatePlayer: true)
backgroundView.snp.makeConstraints {
$0.edges.equalToSuperview()
}
Expand Down Expand Up @@ -394,7 +405,6 @@ class NewTabPageViewController: UIViewController {
self?.reportSponsoredBackgroundEvent(.viewedImpression)
}

setupBackgroundVideoIfNeeded()
videoPlayer?.loadAndAutoplayVideoAssetIfNeeded(
shouldAutoplay: shouldShowBackgroundVideo()
)
Expand Down Expand Up @@ -487,20 +497,22 @@ class NewTabPageViewController: UIViewController {
)
}

func setupBackgroundVideoIfNeeded() {
func setupBackgroundVideoIfNeeded(shouldCreatePlayer: Bool) {
videoButtonsView.isHidden = true

// Setup the background video only if:
// - it hasn't been setup before
// - the current NTP background is a video
guard videoPlayer == nil,
let backgroundVideoPath = background.backgroundVideoPath
else {
guard let backgroundVideoPath = background.backgroundVideoPath else {
videoPlayer = nil
backgroundView.resetPlayerLayer()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to free resources and hide play button when the current NTP is changed from NTT video to image background.

backgroundButtonsView.resetVideoBackgroundButtons()
return
}

gradientView.isHidden = false
videoPlayer = NewTabPageVideoPlayer(backgroundVideoPath)
if shouldCreatePlayer {
videoPlayer?.createPlayer()
}

backgroundView.setupPlayerLayer(backgroundVideoPath, player: videoPlayer?.player)

videoButtonsView.tappedBackgroundVideo = { [weak videoPlayer] in
Expand Down
Loading