mlt icon indicating copy to clipboard operation
mlt copied to clipboard

Published 20 hours ago • verified publisher icon mltframework

mlt_playlist fixes

Open max-verem opened this issue 6 years ago • 5 comments

I did some tests with a playlist item modification and reordering and found some issues

First issue related to mlt_playlist_move function: current == dest special case is wrong. Fix is next:

From 586fb28531282a8be64286f90e5177ae772d5452 Mon Sep 17 00:00:00 2001
From: Maksym Veremeyenko <[email protected]>
Date: Tue, 25 Dec 2018 17:59:11 +0200
Subject: [PATCH] Fix playlist move current positioning

---
 src/framework/mlt_playlist.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/framework/mlt_playlist.c b/src/framework/mlt_playlist.c
index 9942e0b..6ed428d 100644
--- a/src/framework/mlt_playlist.c
+++ b/src/framework/mlt_playlist.c
@@ -930,12 +930,10 @@ int mlt_playlist_move( mlt_playlist self, int src, int dest )
 
 		if ( current == src )
 			current = dest;
-		else if ( src < current && current < dest )
+		else if ( src < current && current <= dest )
 			current --;
-		else if ( dest < current && current < src )
+		else if ( dest <= current && current < src )
 			current ++;
-		else if ( current == dest )
-			current = src;
 
 		src_entry = self->list[ src ];
 		if ( src > dest )
-- 
1.8.3.1

Next issue related to changing repeat playlist item attribute. After changing it playlist current position become wrong. Fix is follow:

From 31a935681f1d0d7d4ef8780711c84aa02c9a9824 Mon Sep 17 00:00:00 2001
From: Maksym Veremeyenko <[email protected]>
Date: Thu, 27 Dec 2018 12:21:30 +0200
Subject: [PATCH] Keep playlist logical position the same after changing same

---
 src/framework/mlt_playlist.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/framework/mlt_playlist.c b/src/framework/mlt_playlist.c
index 6ed428d..05b8544 100644
--- a/src/framework/mlt_playlist.c
+++ b/src/framework/mlt_playlist.c
@@ -970,9 +970,17 @@ int mlt_playlist_repeat_clip( mlt_playlist self, int clip, int repeat )
 	int error = repeat < 1 || clip < 0 || clip >= self->count;
 	if ( error == 0 )
 	{
+		int current = mlt_playlist_current_clip( self );
+		mlt_position position = mlt_producer_position( MLT_PLAYLIST_PRODUCER( self ) );
+		// We need all the details about the current clip
+		mlt_playlist_clip_info current_info;
+		mlt_playlist_get_clip_info( self, &current_info, current );
+		position = (int)(position - current_info.start) % (current_info.frame_count / current_info.repeat);
 		playlist_entry *entry = self->list[ clip ];
 		entry->repeat = repeat;
 		mlt_playlist_virtual_refresh( self );
+		mlt_playlist_get_clip_info( self, &current_info, current );
+		mlt_producer_seek( MLT_PLAYLIST_PRODUCER( self ), current_info.start + position );
 	}
 	return error;
 }
-- 
1.8.3.1

Please review and apply.

max-verem avatar Jan 02 '19 12:01 max-verem

I want unit tests for changes in this area that shows the expected behavior with failing tests due to the current bugs. They should be added in a new directory in src/tests and use QTest, of course. Also, submission in the form of a github pull request is appreciated.

On Wed, Jan 2, 2019 at 4:17 AM Maksym Veremeyenko [email protected] wrote:

I did some tests with a playlist item modification and reordering and found some issues

First issue related to mlt_playlist_move function: current == dest special case is wrong. Fix is next:

From 586fb28531282a8be64286f90e5177ae772d5452 Mon Sep 17 00:00:00 2001 From: Maksym Veremeyenko [email protected] Date: Tue, 25 Dec 2018 17:59:11 +0200 Subject: [PATCH] Fix playlist move current positioning

src/framework/mlt_playlist.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/framework/mlt_playlist.c b/src/framework/mlt_playlist.c index 9942e0b..6ed428d 100644 --- a/src/framework/mlt_playlist.c +++ b/src/framework/mlt_playlist.c @@ -930,12 +930,10 @@ int mlt_playlist_move( mlt_playlist self, int src, int dest )

  if ( current == src )
  	current = dest;

  • else if ( src < current && current < dest )

  • else if ( src < current && current <= dest )
	current --;

  • else if ( dest < current && current < src )

  • else if ( dest <= current && current < src )
	current ++;

  • else if ( current == dest )

  • 	current = src;

src_entry = self->list[ src ];
if ( src > dest )

-- 1.8.3.1

Next issue related to changing repeat playlist item attribute. After changing it playlist current position become wrong. Fix is follow:

From 31a935681f1d0d7d4ef8780711c84aa02c9a9824 Mon Sep 17 00:00:00 2001 From: Maksym Veremeyenko [email protected] Date: Thu, 27 Dec 2018 12:21:30 +0200 Subject: [PATCH] Keep playlist logical position the same after changing same

src/framework/mlt_playlist.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/src/framework/mlt_playlist.c b/src/framework/mlt_playlist.c index 6ed428d..05b8544 100644 --- a/src/framework/mlt_playlist.c +++ b/src/framework/mlt_playlist.c @@ -970,9 +970,17 @@ int mlt_playlist_repeat_clip( mlt_playlist self, int clip, int repeat ) int error = repeat < 1 || clip < 0 || clip >= self->count; if ( error == 0 ) {

  • int current = mlt_playlist_current_clip( self );

  • mlt_position position = mlt_producer_position( MLT_PLAYLIST_PRODUCER( self ) );

  • // We need all the details about the current clip

  • mlt_playlist_clip_info current_info;

  • mlt_playlist_get_clip_info( self, &current_info, current );

  • position = (int)(position - current_info.start) % (current_info.frame_count / current_info.repeat);
playlist_entry *entry = self->list[ clip ];
entry->repeat = repeat;
mlt_playlist_virtual_refresh( self );

  • mlt_playlist_get_clip_info( self, &current_info, current );

  • mlt_producer_seek( MLT_PLAYLIST_PRODUCER( self ), current_info.start + position );
    } return error; } -- 1.8.3.1

Please review and apply.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mltframework/mlt/issues/396, or mute the thread https://github.com/notifications/unsubscribe-auth/ABF_O9O5rfvatmdvrkF-A4AqgmFozhlKks5u_KNzgaJpZM4Zmksn .

-- +-DRD-+

ddennedy avatar Jan 02 '19 17:01 ddennedy

There are now unit tests in src/tests/test_playlist/test_playlist.cpp. Add test cases that shows the failure on the current code, and submit a pull request that shows your commit fixes it.

ddennedy avatar Mar 10 '19 20:03 ddennedy

Here are the commands our CI system runs to execute the tests. Might save you a few seconds of googling: https://github.com/mltframework/mlt-scripts/blob/master/test/test_qtest.sh#L96 https://github.com/mltframework/mlt-scripts/blob/master/test/test_qtest.sh#L111

bmatherly avatar Mar 10 '19 21:03 bmatherly

I dont understand how test_playlist.cpp analyze current playing position. Patches i provided related to fixing wrong playback position on playlist shuffled/modified...

max-verem avatar Mar 11 '19 07:03 max-verem

Modify this file to add a new test function: https://github.com/mltframework/mlt/blob/master/src/tests/test_playlist/test_playlist.cpp

The function should create a playlist (add elements as necessary), read the current position, make modifications and then read the current position again to demonstrate the problem. The test should fail.

Then, apply your patch and see that the test passes.

bmatherly avatar Apr 01 '19 03:04 bmatherly