Enhancements: Start Time Initialization and Extended Volume Range

[itsSagarBro]

Pull Request Description

Summary

This pull request introduces several enhancements to the Flutter VLC Player, including the ability to specify a start time when initializing a video and the option to set and get the maximum volume up to 200%. These improvements enhance the player's functionality and provide more flexibility for developers working with video playback in Flutter applications.

Changes Made

1. Start Time Initialization: Added the startAt parameter to the player's initialization, allowing developers to specify a start time in duration when opening a video or setting media. This enables users to jump to a specific point in the video when it starts playing.

2. Extended Volume Range: Enhanced the volume control by allowing volumes up to 200% (2x). This added flexibility in volume control can be useful for scenarios where users want to amplify audio beyond the standard 100% volume.

How to Use the New Features

Start Time Initialization

VlcPlayer(
  controller: VlcPlayerController.network(
    'your_video_url_here',
    startAt: const Duration(seconds: 5), // Start playing the video at 5 seconds.
  ),
)

Extended Volume Range

To set the volume to 150%:

vlcController.setMaxVolume(150);

To get the current volume:

double currentVolume = vlcController.getMaxVolume();

Testing

I have thoroughly tested these changes by creating unit tests and running sample Flutter applications to ensure that the new features work as expected. No regressions were identified during testing.

[illia-romanenko]

Hey @itsSagarBro , thanks for the PR - I'll try to ask somebody to review it sometime soon(-ish).

[ghost]

@itsSagarBro Thank you for the PR.
I have a few comments:

• I noticed that if the 'startAt' value is set to a value greater than the video's length, the video won't play.
• I’m not sure if we need to change '_maxVolume' to 200 because it doesn't seem to affect the volume, which I believe was the intended purpose. This value appears to only add a number of steps.

I've also added some comments in the code, please take a look

[Ahtsham0715]

Unhandled Exception: Invalid argument (Start At cannot be greater than video duration.): Instance of 'Duration'.

[illia-romanenko]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces valuable enhancements: start time initialization and an extended volume range up to 200%. The changes are generally well-implemented and clearly described.

The addition of startAt provides useful flexibility for video playback, and the extended volume control is a good feature for users needing audio amplification.

I have one suggestion regarding the _setStartAt method to improve its robustness, particularly when video duration might initially be reported as zero. The rest of the changes look good.

Summary of Findings

• Robustness of _setStartAt duration check: The _setStartAt method's check against value.duration might prematurely throw an error if the video duration is initially Duration.zero while startAt is positive. This could prevent the startAt feature from working reliably in scenarios where duration information is delayed. A suggestion was made to allow seeking if the duration is unknown, making the check conditional on value.duration > Duration.zero.
• Redundant null-check in _setStartAt: In _setStartAt, the startAt variable is assigned using ?? Duration.zero after a preceding check already ensures it's non-null. Using startAt! would be slightly cleaner. (This was noted but not created as a separate comment due to review severity settings, but included in the body of the main comment for this section).

Merge Readiness

The pull request introduces significant and useful features: start time initialization and extended volume range. The implementation is largely solid and well-documented.

One medium-severity issue has been identified in the _setStartAt method concerning its behavior when video duration is initially reported as zero. It's recommended to address this to ensure the startAt feature is robust across different scenarios, especially for content where duration might not be immediately available.

Once this point is considered and addressed, the PR should be in excellent shape for merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by authorized team members before merging.

[gemini-code-assist]

There are a couple of points regarding the _setStartAt method logic here:

1. Nullable startAt Assignment (Minor): On line 358, startAtDuration is assigned using startAt ?? Duration.zero. However, the check on line 356 (if (startAt == null || _startAtSet) return;) ensures that startAt (which refers to this.startAt from the constructor) is not null if the function proceeds to this point. Using startAt! would be more direct and reflect this guarantee: final Duration startAtDuration = startAt!;.

2. Duration Check Robustness (Main Point): The check on lines 361-365 (if (startAtDuration.inMilliseconds > value.duration.inMilliseconds)) will throw an ArgumentError. This is problematic if value.duration is Duration.zero (e.g., for live streams, or if duration metadata isn't immediately available when the playing event fires) while startAtDuration is positive. This could unnecessarily prevent the startAt feature from working in these scenarios.

   To make this more robust, consider only throwing the error if value.duration is known (i.e., non-zero) and startAtDuration exceeds it. If value.duration is zero, it might be better to proceed with the seekTo call and let the underlying player handle the seek attempt. This would allow startAt to function even if the duration is temporarily unknown.

   Here's a suggested modification incorporating both points:

   // In _setStartAt()
   final Duration startAtDuration = startAt!; // Point 1 addressed
   
   // Only perform the check if the video's duration is known (greater than zero)
   if (value.duration > Duration.zero && 
       startAtDuration.inMilliseconds > value.duration.inMilliseconds) {
     throw ArgumentError.value(
       startAtDuration,
       'Start At cannot be greater than video duration.',
     );
   }
   // ... rest of the method ...

   This change allows the seek to be attempted if the duration is initially unknown, potentially improving the reliability of the startAt feature.

    final Duration startAtDuration = startAt!;

    // Checking if startAt is greatet then video's duration then throw error.
    // Only throw if duration is known (non-zero) and startAt exceeds it.
    if (value.duration > Duration.zero && 
        startAtDuration.inMilliseconds > value.duration.inMilliseconds) {
      throw ArgumentError.value(
        startAtDuration,
        'Start At cannot be greater than video duration.',
      );
    }