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

refactor(ui): replace package:dart_vlc with package:media_kit #1743

Closed
wants to merge 14 commits into from

Conversation

alexmercerind
Copy link

Submit a pull request

CLA

  • I have signed the Stream CLA (required).
  • The code changes follow best practices
  • Code changes are tested (add some information if not applicable)

Description of the pull request

This PR replaces dart_vlc with media_kit (used on Microsoft Windows & GNU/Linux in stream_chat_flutter). media_kit has major benefits over dart_vlc e.g. better stability, improved performance, less bundle size etc.

To simplify & reduce the total size of implementation, video_player_media_kit is used. It automatically adds support for unsupported platforms to video_player. This means existing video_player-based implementation is automatically shared between all platforms without any additional implementation, wrapper or boilerplate.

video_player_media_kit is part of media_kit project itself, where it receives similar support & maintenance.

It also happens to be sponsored by GetStream.IO!

References:

@alexmercerind alexmercerind changed the title refactor: replace package:dart_vlc with package:media_kit refactor(ui): replace package:dart_vlc with package:media_kit Oct 6, 2023
@alexmercerind
Copy link
Author

I tested the stream_chat_flutter/example app on Windows & GNU/Linux after changes, videos seem to play just fine.

name: stream_chat_flutter_example
description: A new Flutter project.
publish_to: none
version: 1.0.0+1

environment:
  sdk: ">=3.0.0 <4.0.0"
  flutter: ">=3.10.0"

dependencies:
  collection: ^1.15.0
  cupertino_icons: ^1.0.4
  flutter:
    sdk: flutter
  responsive_builder: ^0.7.0
  stream_chat_flutter:
    path: ../../stream_chat_flutter
  stream_chat_localizations:
    path: ../../stream_chat_localizations
  stream_chat_persistence:
    path: ../../stream_chat_persistence

dependency_overrides:
  stream_chat_flutter:
    path: ../../stream_chat_flutter
  stream_chat_localizations:
    path: ../../stream_chat_localizations
  stream_chat_persistence:
    path: ../../stream_chat_persistence

flutter:
  uses-material-design: true
  assets:
    - assets/background_doodle.png

Copy link
Contributor

@esarbanis esarbanis left a comment

Choose a reason for hiding this comment

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

I really think we should first deprecate FullScreenMedia* classes before removing them. They are part of our public interface, and if we remove them like that. will probably break users' apps.

packages/stream_chat_flutter/lib/src/stream_chat.dart Outdated Show resolved Hide resolved
@alexmercerind
Copy link
Author

I really think we should first deprecate FullScreenMedia* classes before removing them. They are part of our public interface, and if we remove them like that. will probably break users' apps.

I hope the changes are according to your expectation.

Thanks!

@Brazol
Copy link
Contributor

Brazol commented Nov 30, 2023

Hi @alexmercerind, we would like to merge this before the next release. I updated the branch with the latest develop, and it caused the tests to fail. Could you take a look at that?

@esarbanis esarbanis changed the base branch from develop to master December 4, 2023 15:09
@esarbanis
Copy link
Contributor

Hey @alexmercerind !

We have switched our main branch to master, and I took the liberty of switching the base of this PR to that. I see there are a few conflicts. Once you can resolve those and the tests pass, I will merge them.

Thank you for bearing with us for so long. I am really sorry for your inconvenience 🙏

@jnelle
Copy link

jnelle commented Feb 21, 2024

@alexmercerind any updates on this?

@esarbanis
Copy link
Contributor

@jnelle we have planned it to be part of our v7.1.0, so we will try to drive it to master by end of next week.

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.

Project coverage is 60.04%. Comparing base (226933c) to head (de4bd62).
Report is 34 commits behind head on master.

Files Patch % Lines
...tter/lib/src/message_widget/parse_attachments.dart 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1743      +/-   ##
==========================================
+ Coverage   59.40%   60.04%   +0.64%     
==========================================
  Files         310      306       -4     
  Lines       17834    17649     -185     
==========================================
+ Hits        10594    10598       +4     
+ Misses       7240     7051     -189     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the Stale label Mar 25, 2024
@IShark01
Copy link

Hello! I created an issue that I believe could be resolved by this change:
#1959

Otherwise, I believe this PR would serve as a fix as well in the dart_vlc repo:
alexmercerind/dart_vlc#388

This is impacting my production app, so it is a relatively urgent issue. Is there another workaround recommended prior to one of these PR's merging?

Also could I get an estimate of when either/both of these PR's will be merged and deployed?

Thanks in advance!

@github-actions github-actions bot removed the Stale label Jun 18, 2024
@github-actions github-actions bot added Stale and removed Stale labels Jul 8, 2024
@github-actions github-actions bot added Stale and removed Stale labels Jul 29, 2024
@github-actions github-actions bot added Stale and removed Stale labels Aug 22, 2024
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.

5 participants