Define image format in producer's get_frame method so we can possibly...

[j-b-m]

Here is my proposal as discussed to define the image format in each producer's get_frame method, before attempting to fetch an image. This allows possible optimizations like I do in qtblend (requesting the best suited image format by default.

I tried to add the change in all possible producers. Some points to be discussed/reviewed: avformat producer: I included a minor refactoring to avoid code duplication color producer: There is some code duplication between get_frame and get_image. Also I set the default format to yuv422 when the color has an opaque alpha component and rgba otherwise, maybe that is not the best. blipflash and noise producers Handle both yuv/rgba cases themselves in get_image so I didn't set the format hold and consumer producers Does not seem possible / useful for these 2 as we don't necessarily know the format at that stage

Let me know ŵhat you think, in any case this leads to some nice performance improvement in qtblend transition.

[j-b-m]

On Freitag, 16. September 2022 22:55:44 CEST Dan Dennedy wrote:

@ddennedy requested changes on this pull request.

Thanks for the review.

@@ -252,6 +252,17 @@ static int producer_get_frame( mlt_producer producer, mlt_frame_ptr frame, int i if ( mlt_properties_get( producer_props, "colour" ) != NULL ) mlt_properties_set( producer_props, "resource", mlt_properties_get( producer_props, "colour" ) );

• char *colorstring = mlt_properties_get( producer_props, 
  

"resource" );

• if ( colorstring && strchr( colorstring, '/' ) )
  

• {
  

• 	colorstring = strdup( strrchr( colorstring, 
  

'/' ) + 1 );

• 	mlt_properties_set( producer_props, 
  

"resource", colorstring );

• 	free( colorstring );
  

• }
  

• mlt_color color = mlt_properties_get_color( 
  

producer_props, "resource" );

• // Inform framework of the default frame format for this 
  

producer

• mlt_properties_set_int( properties, "format", color.a < 
  

255 ?

mlt_image_rgba : mlt_image_yuv422 );

This does not factor in the mlt_image_format property on the color producer.

Yes, I thought about it after my initial commit, will fix that.

@@ -25,6 +25,16 @@

#include <QPainter> #include <QTransform>

+static int is_opaque( uint8_t *image, int width, int height )

Since this appears in transition_frei0r.c I think it should be refactored into the mlt_image class simply for convenience but not to require a mlt_image struct as that would be inefficient. Brian has a goal to include mlt_image pointers on a new get_image callback someday. So, we need to anticipate that someday there will be a mlt_image_is_opaque() that takes a mlt_image pointer. Thus, lets call this one mlt_image_rgba_opaque().

Also, Brian and I recently learned that compilers can optimize a for loop with buffer array indexing better than a while loop with pointer arithmetic. So, we ought to refactor for that as well: ``` int mlt_image_rgba_opaque(uint8_t *image, int width, int height) { for (int i = 0; i < width * height; ++i) { if (image[4 * i + 3] != 0xff) return 0; } return 1; }

And do not forget to add it to `mlt.vers`.

Ok, will do.

@@ -56,10 +66,10 @@ static int get_image( mlt_frame a_frame, uint8_t **image, mlt_image_format *form bool distort = mlt_properties_get_int( transition_properties, "distort" );

// Potention optimization if the producers do set their native format before fetching image - /*if (mlt_properties_get_int( b_properties, "format" ) == mlt_image_rgba) {

What is the status of this commented-out code?

This is the part that provides a nice speedup when compositing a producer that provides rgba image by default, I can change the comment to remove "Potential".

I will be on vacation until the 25th of september, will work on implementing your suggestions as soon as I am back.

Best regards,

Jean-Baptiste

[bmatherly]

Do you think this documentation is still sufficient? https://github.com/mltframework/mlt/blob/master/src/framework/mlt_frame.h#L68 I wonder if we should add some additional text to explain that before an image is retrieved the property represents the native format of the producer.

Can you provide some melt command line examples that I can use to test the performance improvement before and after the change?

[j-b-m]

So I have now updated my pull request taking Dan's comment into account. Here is a short overview of the performance gain and commands used on my system (10 years old intel i7 with 4 cores 8 threads). All tests were performed twice to get a mean time, using full HD sources (videos and images).

Compositing an image with alpha over a YUV420p video: time melt sample_video.mp4 out=2000 -track picture.png out=2000 -transition qtblend -consumer avformat:test.mp4

qtblend (before the change): 2m00s

qtblend with my "format" optimization: 1m41s

frei0r.cairoblend: 1m40s

Compositing an image without alpha (jpeg) over a YUV420p video: time melt sample_video.mp4 out=2000 -track picture.jpeg out=2000 -transition qtblend -consumer avformat:test.mp4

qtblend (before the change): 0m14s

qtblend with my "format" optimization: 0m14s

frei0r.cairoblend: 1m40s

Compositing two YUV420p videos (without alpha): time melt sample_video.mp4 out=2000 -track melt sample_video.mp4 out=2000 -transition qtblend -consumer avformat:test.mp4

qtblend (before the change): 1m23s

qtblend with my "format" optimization: 1m23s

frei0r.cairoblend: 1m34s

[ddennedy]

FYI @j-b-m I reverted the changes I made in transition_frei0r.c to take advantage of this new work due to this bug: https://forum.shotcut.org/t/clip-transition-applies-to-lower-layers/34878 Basically mlt_image_rgba_opaque() is naive, and does not factor in transparent padding added in filter resize. I guess you can add more code (see filter_resize.c) to check if the image will be padded or not.

[j-b-m]

@ddennedy thanks for letting me know. If I am not mistaken, we avoid the problem in qtblend by checking if the display aspect ratio of the b-frame is different from profile and enforce alpha in that case: https://github.com/mltframework/mlt/blob/813fa681798c11a32229f5fbcc8127fca31aad73/src/modules/qt/transition_qtblend.cpp#L173

But I can make further tests...