Created attachment 364335 [details] [review]
CineForm plugin

Created attachment 364336 [details] [review]
Add CineForm plugin to gst-plugins-bad build

(In reply to emeric.grange from comment #0)

> The pixel formats used internally by the SDK are not mapping very well with
> the ones from GStreamer, so the decoder plugin output regular 8b RGB and the
> encoder can take YUY2 or r210 buffers, but it is planned to go full b64a
> (ARGB64_BE) to avoid unnecessary conversions and take advantage of the 10-12
> bits per pixel without subsampling of the CineForm coding.

Apart from endianness-aware ARGB64, what other formats would be good to have for CineForm? Are there any worth being added to GStreamer for example?

Is CineForm always internally operating on some kind of (A)RGB, or can it also operate on some kind of (A)YUV without the conversion between the two?

Review of attachment 364335 [details] [review]:

I would just call the directory cineform, shorter and the SDK part does not really add anything.

Also generally, don't use C99-style comments (//) but the old /* */ ones :)


Apart from that, just a short review for now

::: ext/cineformsdk/cineform_allocator.c
@@ +30,3 @@
+{
+    (void)allocator; // custom UNUSED macro
+    return _mm_malloc(size, alignment);

gst_allocator_alloc() with NULL as allocator and the appropriate parameters might help here, to make it more platform independent? Do you need just memory or is memory wrapped in something else possible?

::: ext/cineformsdk/cineform_allocator.h
@@ +30,3 @@
+
+void *AllocateBuffer(size_t size);
+void DeallocateBuffer(void *buffer);

It would be best to a) give these functions some proper namespace (prefix), and b) mark them as G_GNUC_INTERNAL. a) is needed so that static linking can work without potential conflict with other libraries. AlignedAlloc() is not really a unique name for example.

::: ext/cineformsdk/cineform_debug.c
@@ +32,3 @@
+        fcc_str[3] = (char)((fcc >> 24) & 0xFF);
+        fcc_str[4] = '\0';
+    }

There's GST_FOURCC_ARGS() and GST_FOURCC_FORMAT, which might help here. But not really a problem

@@ +73,3 @@
+            for (unsigned j = 0; j < linesize; j++)
+            {
+                printf("%02X ", (bufferData[i+j]));

gst_util_dump_mem()? Also how is this used, printing to stdout is not nice for a plugin, unless it's optional :)

::: ext/cineformsdk/cineform_debug.h
@@ +34,3 @@
+const char *getErrorString(const CFHD_Error error);
+void printEncodedFormats(const CFHD_EncodedFormat format);
+void printEncodingFormats(const unsigned level, const CFHD_EncodedFormat format);

Same here as with cineform_allocator.h

::: ext/cineformsdk/gstcineformdec.c
@@ +140,3 @@
+    g_object_class_install_property (gobject_class, PROP_DECODING_FORMAT,
+                                     g_param_spec_uint ("decoding-format", "Decoding format",
+                                                        "Decoding format",

What is this?

@@ +146,3 @@
+    g_object_class_install_property (gobject_class, PROP_DECODING_SIZE,
+                                     g_param_spec_uint ("decoding-size", "Decoding size",
+                                                        "Decoding size",

And this?

@@ +170,3 @@
+    g_object_class_install_property (gobject_class, PROP_STEREO_FLAGS,
+                                     g_param_spec_flags ("stereo-flags", "Stereo Flags",
+                                                         "Flags to control stereo processing",

Should the stereo things be taken from the caps instead maybe? What are they?

@@ +382,3 @@
+
+static GstFlowReturn
+gst_cfhd_dec_image_to_buffer (GstCFHDDec * dec, GstVideoCodecFrame * frame)

Can we have the decoder output directly to our frame? memcpy() is expensive :)

@@ +403,3 @@
+        vmeta = gst_buffer_get_video_meta (frame->output_buffer);
+        if (!vmeta)
+          vmeta = gst_buffer_add_video_meta (frame->output_buffer,

This would usually be done directly from the buffer pool already, if you enable it during decide_allocation()

@@ +516,3 @@
+    if (deadline < 0)
+    {
+        GST_WARNING("---- gst_video_decoder_drop_frame() -- late frame dropping");

You should've probably dropped it already before decoding then?

::: ext/cineformsdk/gstcineformenc.c
@@ +50,3 @@
+//      ABGR, BGRA, ARGB64 (A > x)
+//      RG24 (top > bottom)
+// NOK: AYUV64, v210

What does this mean? All those formats are in theory supported by GStreamer, depending on details what they actually mean for CFHD :)

@@ +129,3 @@
+                                                        "Encoding format",
+                                                        0, 4, DEFAULT_ENCODING_FORMAT,
+                                                        G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

This should probably get an enum and become an enum property? Does the format have to be part of the caps, i.e. should it be negotiated?

@@ +353,3 @@
+    gst_cfhd_enc->videoWidth = info->width;
+    gst_cfhd_enc->videoHeight = info->height;
+    gst_cfhd_enc->videoChannels = 1; // use info->ABI.abi.multiview_mode

If you store the input_state, no need to store any of the above additionally. The state contains it all

@@ +444,3 @@
+    gst_buffer_append_memory (buffer_out, mem);
+
+    gst_video_frame_unmap (&vframe);

Ideally you'd use gst_video_encoder_allocate_output_buffer() here, and ideally we could have the encoder directly write into our memory instead of memcpy() :) Instead of memcpy() you can also use gst_buffer_fill() btw (which is a memcpy() internally, but you can prevent the map/copy/unmap dance)

@@ +459,3 @@
+gst_cfhd_enc_propose_allocation (GstVideoEncoder * encoder, GstQuery * query)
+{
+    gst_query_add_allocation_meta (query, GST_VIDEO_META_API_TYPE, NULL);

This would mean that your encode can handle *any* rowstride, plane padding, etc. Can it? You need to get it from the GstVideoFrame for each frame

@@ +468,3 @@
+
+gboolean
+cfhd_open(GstCFHDEnc *gst_cfhd_enc)

Helper functions should be static, and maybe add some kind of gst_ namespace prefix here so that it's immediately clear from the caller site that it's not something from the SDK

@@ +532,3 @@
+    {
+        printf("* Video width is not a multiple of 16. Fixing that.\n");
+        gst_cfhd_enc->internalBufferWidth = ceil((float)(gst_cfhd_enc->videoWidth) / 16.0) * 16;

GST_ROUND_UP_16()?

@@ +541,3 @@
+    {
+        printf("* Video height is not a multiple of 8. Fixing that.\n");
+        gst_cfhd_enc->internalBufferHeight = ceil((float)(gst_cfhd_enc->videoHeight) / 8.0) * 8;

GST_ROUND_UP_8()?

Related to the pixel formats, we added BGR/A 10 and 12 bits with your help last march I believe, but did not started using them because we were already shipping an older GStreamer version. I think these are the closest to internal pixel format used by the codec, and it was always planned to transition to them.

Internally you can have YUV 422 10 bits, RGB 444 12 bits and RGBA 4444 12 bits. There is a way to get what mode is used by a video.

The pixel formats supported by the SDK are specified here: https://github.com/gopro/cineform-sdk/blob/master/Common/CFHDTypes.h but many of them are not corresponding to what you would expect them to describe. I'm not sure what formats are native (or close enough), and what formats use an internal converter.

+ I would just call the directory cineform, shorter and the SDK part does not really add anything.
You mean for the plugin name & directory? I though we might see another implementation someday, not using this SDK. But I really don't mind if you prefer without the SDK ^^

No problem I'll fix the C99 comments, the cineform_allocator and remove most of the leftover debug code from cineform_debug.c and some ENABLE_STATS.

Regarding "Decoding format", you can force a pixel format. But yeah this is probably not something useful to expose in the context of GStreamer, I'll remove it.

"Decoding size" is actually fun, because CineForm is a wavelet codec, you can ask a decode at full, half or quarter resolution.

+ Should the stereo things be taken from the caps instead maybe?
Definitely.

+ This should probably get an enum and become an enum property? Does the format have to be part of the caps, i.e. should it be negotiated?
While the format could be negotiated I still think the user should choose what he wants.

+ Do you need just memory or is memory wrapped in something else possible?
Just memory.

+ This would mean that your encode can handle *any* rowstride, plane padding, etc.
+ Can it? You need to get it from the GstVideoFrame for each frame
I don't think it can.

I don't remember why I was not encoding directly in memory, because that's the only case where I could actually handle two pixel formats without extra manual conversion. I'll have another take at it.

I'll work on all that and get back to you, thanks for the review!

I tried that patchset to benchmark perfs/how it works for the potential use of CineForm as possible proxy format in the Pitivi Video Editor, but I can not build your patchset right now.

First I needed https://github.com/gopro/cineform-sdk/pull/21 to package for arch and then building fails:

```
ninja: Entering directory `/home/thiblahute/devel/gstreamer/gst-build/build'
[15/377] Compiling C object 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/cineform_debug.c.o'.
FAILED: subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/cineform_debug.c.o 
ccache cc  -Isubprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha -Isubprojects/gst-plugins-bad/ext/cineformsdk -I../subprojects/gst-plugins-bad/ext/cineformsdk -Isubprojects/gst-plugins-bad -I../subprojects/gst-plugins-bad -Isubprojects/gst-plugins-base/gst-libs -I../subprojects/gst-plugins-base/gst-libs -Isubprojects/gstreamer/libs -I../subprojects/gstreamer/libs -Isubprojects/gstreamer -I../subprojects/gstreamer -Isubprojects/gst-plugins-base/gst-libs/gst/video -Isubprojects/gstreamer/libs/gst/base -Isubprojects/gstreamer/gst -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/orc-0.4 -I/usr/include/cineformsdk -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -O0 -g -fvisibility=hidden -fPIC -pthread -DHAVE_CONFIG_H -MMD -MQ 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/cineform_debug.c.o' -MF 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/cineform_debug.c.o.d' -o 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/cineform_debug.c.o' -c ../subprojects/gst-plugins-bad/ext/cineformsdk/cineform_debug.c
../subprojects/gst-plugins-bad/ext/cineformsdk/cineform_debug.c: In function ‘printEncodedFormats’:
../subprojects/gst-plugins-bad/ext/cineformsdk/cineform_debug.c:174:5: error: enumeration value ‘CFHD_ENCODED_FORMAT_UNKNOWN’ not handled in switch [-Werror=switch]
     switch (format)
     ^~~~~~
cc1: all warnings being treated as errors
[22/377] Compiling C object 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformsdk.c.o'.
FAILED: subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformsdk.c.o 
ccache cc  -Isubprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha -Isubprojects/gst-plugins-bad/ext/cineformsdk -I../subprojects/gst-plugins-bad/ext/cineformsdk -Isubprojects/gst-plugins-bad -I../subprojects/gst-plugins-bad -Isubprojects/gst-plugins-base/gst-libs -I../subprojects/gst-plugins-base/gst-libs -Isubprojects/gstreamer/libs -I../subprojects/gstreamer/libs -Isubprojects/gstreamer -I../subprojects/gstreamer -Isubprojects/gst-plugins-base/gst-libs/gst/video -Isubprojects/gstreamer/libs/gst/base -Isubprojects/gstreamer/gst -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/orc-0.4 -I/usr/include/cineformsdk -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -O0 -g -fvisibility=hidden -fPIC -pthread -DHAVE_CONFIG_H -MMD -MQ 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformsdk.c.o' -MF 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformsdk.c.o.d' -o 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformsdk.c.o' -c ../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk.c
In file included from ../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformenc.h:30:0,
                 from ../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk.c:27:
/usr/include/cineformsdk/CFHDEncoder.h:322:6: error: unknown type name ‘bool’; did you mean ‘_Bool’?
      bool temporary);
      ^~~~
      _Bool
[23/377] Compiling C object 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformenc.c.o'.
FAILED: subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformenc.c.o 
ccache cc  -Isubprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha -Isubprojects/gst-plugins-bad/ext/cineformsdk -I../subprojects/gst-plugins-bad/ext/cineformsdk -Isubprojects/gst-plugins-bad -I../subprojects/gst-plugins-bad -Isubprojects/gst-plugins-base/gst-libs -I../subprojects/gst-plugins-base/gst-libs -Isubprojects/gstreamer/libs -I../subprojects/gstreamer/libs -Isubprojects/gstreamer -I../subprojects/gstreamer -Isubprojects/gst-plugins-base/gst-libs/gst/video -Isubprojects/gstreamer/libs/gst/base -Isubprojects/gstreamer/gst -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/orc-0.4 -I/usr/include/cineformsdk -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -O0 -g -fvisibility=hidden -fPIC -pthread -DHAVE_CONFIG_H -MMD -MQ 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformenc.c.o' -MF 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformenc.c.o.d' -o 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformenc.c.o' -c ../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformenc.c
In file included from ../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformenc.h:30:0,
                 from ../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformenc.c:34:
/usr/include/cineformsdk/CFHDEncoder.h:322:6: error: unknown type name ‘bool’; did you mean ‘_Bool’?
      bool temporary);
      ^~~~
      _Bool
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformenc.c:459:1: error: ‘gst_cfhd_enc_propose_allocation’ defined but not used [-Werror=unused-function]
 gst_cfhd_enc_propose_allocation (GstVideoEncoder * encoder, GstQuery * query)
 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
[24/377] Compiling C object 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformdec.c.o'.
FAILED: subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformdec.c.o 
ccache cc  -Isubprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha -Isubprojects/gst-plugins-bad/ext/cineformsdk -I../subprojects/gst-plugins-bad/ext/cineformsdk -Isubprojects/gst-plugins-bad -I../subprojects/gst-plugins-bad -Isubprojects/gst-plugins-base/gst-libs -I../subprojects/gst-plugins-base/gst-libs -Isubprojects/gstreamer/libs -I../subprojects/gstreamer/libs -Isubprojects/gstreamer -I../subprojects/gstreamer -Isubprojects/gst-plugins-base/gst-libs/gst/video -Isubprojects/gstreamer/libs/gst/base -Isubprojects/gstreamer/gst -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/orc-0.4 -I/usr/include/cineformsdk -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -O0 -g -fvisibility=hidden -fPIC -pthread -DHAVE_CONFIG_H -MMD -MQ 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformdec.c.o' -MF 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformdec.c.o.d' -o 'subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformsdk@sha/gstcineformdec.c.o' -c ../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c: In function ‘gst_cfhd_dec_class_init’:
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:143:72: error: multi-character character constant [-Werror=multichar]
                                                         0, 0xFFFFFFFF, DEFAULT_DECODING_FORMAT,
                                                                        ^~~~~~~~~~~~~~~~~~~~~~~
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c: In function ‘gst_cfhd_dec_init’:
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:234:49: error: multi-character character constant [-Werror=multichar]
     gst_cfhd_dec->decFormat = (CFHD_PixelFormat)DEFAULT_DECODING_FORMAT;
                                                 ^~~~~~~~~~~~~~~~~~~~~~~
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c: In function ‘gst_cfhd_dec_image_to_buffer’:
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:430:9: error: unused variable ‘width’ [-Werror=unused-variable]
     int width = GST_VIDEO_FRAME_COMP_WIDTH (&vframe, 0);
         ^~~~~
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:428:11: error: unused variable ‘pixel_stride’ [-Werror=unused-variable]
     guint pixel_stride = GST_VIDEO_FRAME_COMP_PSTRIDE (&vframe, 0);
           ^~~~~~~~~~~~
In file included from ../subprojects/gstreamer/gst/gst.h:55:0,
                 from ../subprojects/gst-plugins-base/gst-libs/gst/video/video.h:23,
                 from ../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:29:
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c: In function ‘gst_cfhd_dec_handle_frame’:
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:458:14: error: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 8 has type ‘uint64_t {aka long unsigned int}’ [-Werror=format=]
     GST_INFO("-------- gst_cfhd_dec_handle_frame()   --   #%llu", gst_cfhd_dec->sampleDecoded);
              ^                                                    ~~~~~~~~~~~~~~~
../subprojects/gstreamer/gst/gstinfo.h:640:31: note: in definition of macro ‘GST_CAT_LEVEL_LOG’
         (GObject *) (object), __VA_ARGS__);    \
                               ^~~~~~~~~~~
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:458:5: note: in expansion of macro ‘GST_INFO’
     GST_INFO("-------- gst_cfhd_dec_handle_frame()   --   #%llu", gst_cfhd_dec->sampleDecoded);
     ^~~~~~~~
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:455:22: error: unused variable ‘gst_cfhd_dec_class’ [-Werror=unused-variable]
     GstCFHDDecClass *gst_cfhd_dec_class = GST_CFHD_DEC_GET_CLASS (gst_cfhd_dec);
                      ^~~~~~~~~~~~~~~~~~
../subprojects/gst-plugins-bad/ext/cineformsdk/gstcineformdec.c:450:10: error: variable ‘decoder_deadline’ set but not used [-Werror=unused-but-set-variable]
     long decoder_deadline = 0;
          ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
ninja: build stopped: subcommand failed.
```

Almost same issues with autotools.

Looks like the headers use some C++ features (bool) but we build as pure C - I tried forcing to build as C++ but I get a new issues doing that :-)

(In reply to Thibault Saunier from comment #7)
> I tried that patchset to benchmark perfs/how it works for the potential use
> of CineForm as possible proxy format in the Pitivi Video Editor, but I can
> not build your patchset right now.

That's pretty cool, hopefully we can have it working soon. I'll publish new versions this week so you can have a go at it.

> First I needed https://github.com/gopro/cineform-sdk/pull/21 to package for
> arch and then building fails:

This one is still a work in progress but we will need it for proper packaging yes. I also did it in order to make an Arch package ^^

> Looks like the headers use some C++ features (bool) but we build as pure C -
> I tried forcing to build as C++ but I get a new issues doing that :-)

Yes I saw this one last week, I will patch it upstream too.

Any possible update on that one? :-)

Ok, so obviously not much has been going on on the CineForm front...
So today I forked the SDK here https://github.com/emericg/libcineform

I'll finish the integration tomorrow I think, so we can have a proper build of this new library. The GStreamer patches should change between the SDK and the new library so you'll be able to test everything this week.

Created attachment 372173 [details] [review]
Add CineForm plugin to gst-plugins-bad build

now use libcineform instead of cineform-sdk

Created attachment 372174 [details] [review]
CineForm plugin

Created attachment 372176 [details] [review]
Add CineForm to cerbero

I rebased all of the patches for GStreamer 1.15 and the plugin now use the https://github.com/emericg/libcineform instead of CineForm-SDK. Same API, only build system work and massive code removal.
I didn't test the 0001-cfhd-buildsystem_v2.patch, so there might just a little renaming left to do.
As far as I remember I didn't do any modifications on the plugin in itself since last time, so I will still have to work on that.

@Thibault Saunier, performances of the CineForm SDK / libcineform seems OK on linux at least, and there is a libcineform pkgbuild if you want to try it.
I want to try to make a nice CineForm feature work, the "half/quarter/eighth
" resolution decoding. That might be useful for your workflow, but so far I never saw a performance boost so, I might be doing something wrong.

I'll be continuing to work on this over the next few days.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/634.