Fix issue with the progress listener when using Clip

[mdolnik]

Q	A
Bug?	yes
New Feature?	no
Version Used	0.18.0
FFmpeg Version	FFmpeg
OS	Windows 10

Actual Behavior

When saving a clip of a video using Video::clip() (not ClipFilter) and using a progress listener, the progress percentage does not output correctly.

The progress hangs around 0-1% then jumps to 50-51% and hangs there until it is finished, then will jump to 100%.

This is more evident on long videos (eg 1 hour +).

This is because in AbstractVideo::save() (unlike ClipFilter) using Clip does not provide the duration value to createProgressListener() so the progress is calculated using the progress of how much of the clip has been processed against the length of the entire video rather than the length of the clip itself.

Expected Behavior

The percentage should be accurate to the progress of the encoding.

Steps to Reproduce

$ffmpeg = FFMpeg\FFMpeg::create();
$video = $ffmpeg->open('video.mpg');
$format = new FFMpeg\Format\Video\X264();
$format->on('progress', function ($video, $format, $percentage) {
    echo "$percentage %";
});
$clip = $video->clip(FFMpeg\Coordinate\TimeCode::fromSeconds(30), FFMpeg\Coordinate\TimeCode::fromSeconds(15));
$clip->save(new FFMpeg\Format\Video\X264(), 'video.avi');

Depending on the length of the clip and the length of the total video, this would result in something like:

0 %
0 %
0 %
0 %
1 %
50 %
50 %
50 %
51 %
100 %

Possible Solutions

This can be resolved by adding:

/**
 * Returns the duration as a TimeCode if one was provided.
 *
 * @return \FFMpeg\Coordinate\TimeCode|null
 */
public function getDuration()
{
  return $this->duration;
}

to \FFMpeg\Media\Clip

And adding:

// If this is the Clip object, extract the duration provided to it.
if ($this instanceof Clip) {
  if ($this->getDuration() !== NULL) {
    $duration = $this->getDuration()->toSeconds();
  }
}

to \FFMpeg\Media\AbstractVideo::save() right after $duration = 0; and before // check the filters of the video...

Here is a patch for the changes above: fix_clip_progress.patch.zip

As ugly as it is to add specific logic for a child class within logic of a parent class, this is the simplest / quickest solution.

Alternatively one could add a getDuration() method to MediaTypeInterface in which all media classes would be required to provide a value for (with the default logic to extract the length of the current medium and the Clip class would return the provided duration). This could result in an API breaking change for anyone who has created their own media type classes.

Note: Both of the above solutions do not take into account situations when NULL is provided as the duration to Clip. In this situation NULL would end up being passed to createProgressListener() causing the same issue. One solution to this situation could be to have Clip::getDuration() calculate the duration between the start point and the end of the video, but due to the TimeCode class not being exactly accurate (#551 #735), this could end up with lost frames at the end of the video.