Fix: Create new texture when video frame size is changed.

[alexmercerind]

[WIP]

• Add VideoResizeCallback to Player::OnVideo listener, to get callback ahead of buffer reallocation (if the frame size is changed).
• Now PlayerRegisterTexture & PlayerUnregisterTexture are renamed to PlayerCreateVideoOutlet & PlayerDeleteVideoOutlet.
• Now method channel call no longer creates texture ahead of time, but when first frame of the video is received.
• If a new media of different video dimensions is opened, the previous texture is deleted & new is created (VideoOutlet::CreateNewTexture), whose texture ID is notified through the method channel.
• Currently UnregisterTexture is causing exception (from VideoOutlet::DeleteTexture).
  • Not sure about the reason.
  • Although I'm sure that I'm NOT deleting the texture ID which was never created.

[alexmercerind]

I decided to just change textureId ValueNotifier<int?> to null before we unregister previous texture & register new one. I was hoping this could avoid the crash, atleast it would avoid "texture not registered."

[jnschulze]

Does the old texture get unregistered before you reallocate the pixel buffer here?

dart_vlc/dartvlc/internal/events.h

Line 146 in 09805e4

video_frame_buffer_.reset(new uint8_t[size]);

That's the critical part. If you reuse the same buffer with the new texture you gain nothing.

Besides, the texture creation / deletion part in the video outlet might need synchronization.
(hint: Dump the thread ID using std::this_thread::get_id() to get a feeling of the different threads involved here. I can think of the VLC video render thread, the Flutter main thread, the flutter raster thread... ).

[jnschulze]

You obviously want to recreate the texture if EITHER the width OR the height changes.

[alexmercerind]

silly mistake. 😅

[alexmercerind]

Does the old texture get unregistered before you reallocate the pixel buffer here?

I added

dart_vlc/dartvlc/internal/events.h

Lines 143 to 145 in 09805e4

	if (video_resize_callback_) {
	video_resize_callback_(video_width_, video_height_);
	}

dart_vlc/windows/dart_vlc_plugin.cc

Lines 81 to 85 in 09805e4

	player->OnVideo([outlet_ptr = it->second.get()](
	uint8_t * frame, int32_t width, int32_t height)
	->void { outlet_ptr->OnVideo(frame, width, height); },
	[outlet_ptr = it->second.get()](int32_t, int32_t)
	->void { outlet_ptr->DeleteTexture(); });

this callback to call VideoOutlet::DeleteTexture before video_frame_buffer_.reset(new uint8_t[size]);.

And I can confirm that a new texture_id_ is created before deletion.

Textures must be unregistered on the the platform thread. https://engine.chinmaygarde.com/protocol_flutter_texture_registry_01-p.html#ae612ec7f2b52cd92d1664b9f62784e0f

Maybe this is the cause. Maybe I have to involve Dart calling VideoOutlet::DeleteTexture & this cannot be done by VLC thread directly imo.

APOLOGIES picked the wrong thing.

[jnschulze]

Textures must be unregistered on the the platform thread.

Where did you get this from?

All texture operations in the texture registrar are thread safe:
https://github.com/flutter/engine/blob/58b1b608c83b624146dc66d98172cac43edaad55/shell/platform/windows/flutter_windows_texture_registrar.cc#L48-L62

[alexmercerind]

All I need to do is unmount Texture before calling UnregisterTexture. Let's see, why is that not happening when I'm already sending nullptr.

[jnschulze]

In the unregistration path, the mutex only protects the map. The actual texture unregistration is performed asynchronously after the texture has been removed from the map. So even after TextureRegistrar::UnregisterTexture returns it's not safe to delete the pixel buffer immediately, as it might still be read by glTexImage2D in shell/platform/windows/external_texture_gl.cc.

See https://github.com/flutter/engine/blob/58b1b608c83b624146dc66d98172cac43edaad55/shell/platform/windows/flutter_windows_texture_registrar.cc#L48-L62

[alexmercerind]

Possibly the best would be to publish today's morning patch (that's critical, idk how old OnVideo was not working sometimes) & change default value of videoDimensions in Player constructor to const VideoDimensions(1920, 1080), preventing buffer reallocation until this is fixed.

[jnschulze]

Possibly the best would be to publish today's morning patch (that's critical, idk how old OnVideo was not working sometimes) & change default value of videoDimensions in Player constructor to const VideoDimensions(1920, 1080), preventing buffer reallocation until this is fixed.

I wouldn't change anything on the Dart side. The buffer size doesn't need to match the visible video resolution. You can allocate a fixed buffer of 4096x4096 etc. and don't reallocate it. It shouldn't require any further changes. Having a larger buffer wouldn't even affect performance, as only the visible portion will get copied (both from VLC into the buffer and from the buffer to the GL_TEXTURE_2D used by Flutter).

[alexmercerind]

@jnschulze

I wouldn't change anything on the Dart side.

Changing C++ code (which will still need to be changed) vs temporarily adding a default argument in Dart.
And, users will still have ability to override the frame size per instance of Player.

Having a larger buffer wouldn't even affect performance, as only the visible portion will get copied.

Having high buffer size has actually caused CPU usage to be higher in my tests.

And, I don't know but I actually tested out different frame sizes when adding dynamic buffer allocation. I'm just unaware how this is coming into sight now... Anyways, it's a problem for now.

[jnschulze]

Having a larger buffer wouldn't even affect performance, as only the visible portion will get copied.

Having high buffer size has actually caused CPU usage to be higher in my tests.

Exactly, because you've changed it for the Player, right? By doing that, you not only increase the buffer size, but make all buffer-related paths in C++ deal with your forced width and height, right?

You might want to take a look at your implementation 😄

dart_vlc/dartvlc/internal/events.h

Lines 119 to 159 in bb55014

	void OnVideoDimensionsCallback() {
	int32_t video_width = 0;
	int32_t video_height = 0;
	if (preferred_video_width_.has_value() &&
	preferred_video_width_.has_value()) {
	video_width = preferred_video_width_.value_or(0);
	video_height = preferred_video_height_.value_or(0);
	} else {
	uint32_t px = 0, py = 0;
	libvlc_video_get_size(vlc_media_player_.get(), 0, &px, &py);
	video_width = static_cast<int32_t>(px);
	video_height = static_cast<int32_t>(py);
	}
	video_dimension_callback_(video_width, video_height);
	if (video_width_ != video_width || video_height_ != video_height) {
	video_width_ = video_width;
	video_height_ = video_height;
	int32_t pitch = video_width * 4;
	int32_t size = video_height * pitch;
	video_frame_buffer_.reset(new uint8_t[size]);
	vlc_media_player_.setVideoCallbacks(
	std::bind(&PlayerEvents::OnVideoLockCallback, this,
	std::placeholders::_1),
	nullptr, std::bind(&PlayerEvents::OnVideoPictureCallback, this,
	std::placeholders::_1));
	vlc_media_player_.setVideoFormatCallbacks(
	[=](char* chroma, uint32_t* w, uint32_t* h, uint32_t* p,
	uint32_t* l) -> int32_t {
	strcpy(chroma, "RGBA");
	*w = video_width;
	*h = video_height;
	*p = pitch;
	*l = video_height;
	return 1;
	},
	nullptr);
	vlc_media_player_.setVideoFormat("RGBA", video_width, video_height,
	pitch);
	}
	}

[alexmercerind]

You might want to take a look at your implementation 😄

Literally only PlayerEvents::OnVideoPictureCallback out of that is called per frame, remaining is just invoked when media starts for the first time.
I believe it's internal libVLC computation cost that depends on buffer size. They were pretty concerned about using video callbacks for frame in the first place. libVLC 4.0.0 will come with direct rendering to OpenGL & zero copy callbacks, for now this is all there is.

[jnschulze]

You might want to take a look at your implementation 😄

Literally only PlayerEvents::OnVideoPictureCallback out of that is called per frame, remaining is just invoked when media starts for the first time.
I believe it's internal libVLC computation cost that depends on buffer size. They were pretty concerned about using video callbacks for frame in the first place. libVLC 4.0.0 will come with direct rendering to OpenGL & zero copy callbacks, for now this is all there is.

Hitesh. VLC doesn't know about your buffer size. Do you tell it the size somewhere? Nope. You tell it the width and height. So VLC expects you to have a buffer that is at LEAST width*height*4 for RGBA. But it doesn't prevent you from having a larger buffer. A larger buffer doesn't implicitly affect performance. What affects performance here is that you report a potentially larger width and height than the actual video has to VLC.

[alexmercerind]

I obviously meant the size that is being copied. The larger the dimensions, the larger the size being copied, and in our case that size is equal to the buffer itself.

I didn't think about pre-defining a large buffer ahead of time & how I can avoid reinitialization. Apologies. I was possibly confused by all the stuff.

[jnschulze]

It's ok.
And as I don't know what the side effects of simple forcing an arbitrary resolution for the Player on the Dart side are, I thought it might be simpler to just allocate a buffer that is sufficiently sized.