Fix melt not allowing profile change from embeded consumer

[j-b-m]

When using melt to render a .mlt playlist file that contains a consumer tag, it does not support profile changes. For example if the consumer contains rescale info (like s=320x200), the rescaling is not correctly passed to the producer. Kdenlive bug: https://bugs.kde.org/show_bug.cgi?id=407678

To reproduce, save the following playlist text in a file playlist.mlt file.
This playlist uses a PAL profile and has a red rectangle scaled to 200x200 with an affine filter. The consumer tag applies a rescale using s=320x200.

To reproduce issue, process the playlist with :

melt playlist.mlt

It will create a render in /tmp/test.mp4

You can see that the affine filter does not correctly apply scaling (it scales to 200x200 but ignores the fact that the consumer rescales to 320x200 using the "s=320x200" option), so it almost fills the frame.

This patch fixes the problem, and in the render, the rectangle is now less than a quarter of the frame size.

<mlt LC_NUMERIC="C" version="6.17.0" title="color:red"> <profile description="DV/DVD PAL" width="720" height="576" progressive="0" sample_aspect_num="16" sample_aspect_den="15" display_aspect_num="4" display_aspect_den="3" frame_rate_num="25" frame_rate_den="1" colorspace="601"/> <consumer f="mp4" g="15" crf="23" progressive="1" target="/tmp/test.mp4" threads="1" real_time="-8" mlt_service="avformat" vcodec="libx264" ab="192k" movflags="+faststart" s="320x200" bf="2" acodec="aac" in="0" out="74"/> <producer id="producer0" in="0" out="100"> <property name="length">15000</property> <property name="eof">pause</property> <property name="resource">red</property> <property name="aspect_ratio">1.06667</property> <property name="mlt_service">color</property> <filter id="filter0"> <property name="background">colour:0</property> <property name="mlt_service">affine</property> <property name="transition.geometry">0 0 200 200</property> </filter> </producer> <playlist id="playlist0"> <entry producer="producer0" in="0" out="100"/> </playlist> <tractor id="tractor0" title="color:red" global_feed="1" in="0" out="100"> <track producer="playlist0"/> </tractor> </mlt>

[ddennedy]

There is code that already exists in mlt_consumer.c (and consumer_avformat.c for the "s" property) that is already supposed to handle this. For example, the following works:
melt color:red -attach affine transition.geometry="0 0 200 200" -consumer sdl2 width=320 height=200

but not
melt color:red out=100 -attach affine transition.geometry="0 0 200 200" -consumer avformat:test.mp4 width=320 height=200

Also, if you replace <consumer/> in the XML with <consumer mlt_service="xml" width="320" height="200"/> you will see that it outputs an updated profile. It appears something is wrong with avformat.

the consumer rescales to 320x200 using the "s=320x200" option

The s property is expanded to width and height properties, which are mlt_consumer (base level) properties that update the profile.

If I add aspect=@320/200 to the <consumer> element, then it works as expected. This is due to a bug in consumer_avformat.c:recompute_aspect_ratio() that assumes aspect has been set. I suggest that you supply an aspect ratio value if you are changing the width and height. More than likely, the user is expecting the display aspect ratio to match the resolution, and sample aspect ratio needs to be updated to conform (square/1.0). If the profile sample aspect ratio is kept, then the display aspect ratio is going to change, for example, in this case: 320x200 [SAR 16:15 DAR 128:75].

--- src/modules/avformat/consumer_avformat.c
+++ src/modules/avformat/consumer_avformat.c
@@ -270,8 +270,9 @@ mlt_consumer consumer_avformat_init( mlt_profile profile, char *arg )
 
 static void recompute_aspect_ratio( mlt_properties properties )
 {
 	double ar = mlt_properties_get_double( properties, "aspect" );
+	if (ar > 0.0) {
 		AVRational rational = av_d2q( ar, 255 );
 		int width = mlt_properties_get_int( properties, "width" );
 		int height = mlt_properties_get_int( properties, "height" );
 	
@@ -286,8 +287,9 @@ static void recompute_aspect_ratio( mlt_properties properties )
 		// Update the profile and properties as well since this is an alias
 		// for mlt properties that correspond to profile settings
 		mlt_properties_set_int( properties, "sample_aspect_num", rational.num );
 		mlt_properties_set_int( properties, "sample_aspect_den", rational.den );
+	}
 }
 
 static void color_trc_from_colorspace( mlt_properties properties )
 {

[ddennedy]

P.S. If you prefer to supply a sample aspect ratio supply properties "sample_aspect_num" and "sample_aspect_den" as integers (real numbers will be truncated). Also, this means you can fix it Kdenlive without waiting for MLT release.

[j-b-m]

Hi Dan, thanks for the answer. However the issue you mention is different than what I reported (maybe I was not clear enough). My point it that when doing:

melt playlist.mlt -consumer avformat:test.mp4 s="320x200"

playlist.mlt is processed with its own profile. For example if the "profile" info contains a 1080p profile, frames are requested at 1080p resolution, then resized by the consumer.

If we do:

melt playlist.mlt
(with an embedded consumer tag with s="320x200"), playlist.mlt is processed with the 320x200 profile.

This means that if we have a filter/transition resizing to 300x300, in the first case it will produce a result using 1/3 of the image, and in the second case it will be bigger than full frame.

My patch allows processing the playlist with its own profile when using embedded consumer tag

[ddennedy]

I understand now. In the first case, the loader producer that melt is using notices the profiles are different and then loads it with the "consumer" producer, which has the normalizing filters to scale. I ran into this use case with Shotcut too, and what I did was add a "multi" parameter to the xml producer to force it to use the "multi" consumer to try to achieve the same thing in a different way. Users are reporting that this introduces an a/v sync problem that I have not yet been able to fix. It seems your change is causing the "consumer" producer to kick in, and that might work better. Thanks, I will test it more soon. I have to look into what happens when the multi consumer is used within the xml producer whether automatically (multiple consumer elements) or explicitly (by parameter) after your change.

[ddennedy]

This looks pretty good except we are leaking the cloned profile here. It might be safe to store as a data property with destructor on the consumer. I will test that.

[ddennedy]

Here is my change on top, and it is working OK (not segfaulting) for me.

--- src/modules/xml/producer_xml.c
+++ src/modules/xml/producer_xml.c
@@ -1149,6 +1149,8 @@ static void on_end_consumer( deserialise_context context, const xmlChar *name )
 				char *id = trim( mlt_properties_get( properties, "mlt_service" ) );
 				mlt_profile consumer_profile = mlt_profile_clone( context->profile );
 				context->consumer = mlt_factory_consumer( consumer_profile, id, resource );
+				mlt_properties_set_data(MLT_CONSUMER_PROPERTIES(context->consumer), "producer_xml.consumer_profile",
+					consumer_profile, 0, (mlt_destructor) mlt_profile_close, NULL);
 				if ( context->consumer )
 				{
 					// Track this consumer

[ddennedy]

During my tests, I also monitored some other aspects related to a/v sync and performance that I want to share with you about this approach. First of all, performance parameter in the form of real_time on the consumer element in the XML is not being relayed to the consumer in the consumer producer. It does appear, however, from a casual test that you can add "realtime=-N" to the melt command line before the XML filename. Secondly, I noticed a/v sync is way off when changing frame rate (in conjunction with resolution or aspect ratio to trigger the usage of the consumer producer). Frame rate conversion was never implemented in the consumer producer, mentioned here in the loader when injecting the consumer producer:
https://github.com/mltframework/mlt/blob/master/src/modules/core/producer_loader.c#L127

The multi consumer, on the other hand, does handle frame rate conversion. xml producer also handles the performance aspect of real_time as an attribute on the first consumer element. I have not yet found a repro case for the reported a/v sync problem, and I did not run into either today while testing specifically for that. If you want to use the multi consumer to do this as I have done add "?multi=1" after the XML filename.

[j-b-m]

Thanks for the detailed answer. I updated my pull request to include the fix for profile leak, works for me too. In the meantime I have applied the multi consumer workaround in Kdenlive, thanks.
There is another small issue related to the multi consumer: the in/out points are not passed on the multi consumer when defined in the MLT playlist's consumer tag. I will send another pull request for this.