[Bug 790793] CineForm plugin for GStreamer bad

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790793

--- Comment #4 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 364335:
 --> (https://bugzilla.gnome.org/review?bug=790793&attachment=364335)

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()?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.