Created attachment 291855 [details] [review]
DirectShow decoder update to 1.x patch

Review of attachment 291855 [details] [review]:

Thanks for the patch! Looks mostly good :)

Ideally you would also port them to GstVideoDecoder and GstAudioDecoder ;)

::: sys/dshowdecwrapper/gstdshowaudiodec.cpp
@@ +214,3 @@
+    gst_buffer_set_size(out_buf, MIN ((unsigned int)size, out_buf_map.size));
+    memcpy (out_buf_map.data, pBuffer, gst_buffer_get_size(out_buf));
+    gst_buffer_unmap(out_buf, &out_buf_map);

You can use gst_buffer_fill() here instead of mapping and memcpy()

@@ +377,3 @@
       GST_DEBUG_FUNCPTR (gst_dshowaudiodec_change_state);
 
+  gst_dshowaudiodec_parent_class = (GstElementClass *) g_type_class_peek_parent (klass);

G_DEFINE_TYPE() does that for you already

@@ +548,3 @@
   }
 
+  return GST_ELEMENT_CLASS (gst_dshowaudiodec_parent_class)->change_state (element, transition);

Usually we just #define gst_dshowaudiodec_parent_class to parent_class before G_DEFINE_TYPE() to prevent typing too much

@@ +630,3 @@
       GST_BUFFER_TIMESTAMP (buffer) + GST_BUFFER_DURATION (buffer),
+      map.size, (bool)discont);
+  gst_buffer_unmap(buffer, &map);

That would've been broken before already, but is it guaranteed that PushBuffer() creates a copy of the memory if it needs it after the function has returned?

@@ +670,1 @@
+      /*HELP: update flag from segment event now gone, is flushing on segment needed?? */

No

@@ +819,3 @@
+        format->cbSize = map.size;
+
+        gst_buffer_unmap(adec->codec_data, &map);

Can use gst_buffer_extract() instead of map() and memcpy()

@@ +882,3 @@
 
+const char*
+gst_dshowaudiodec_format_from_depth(gint depth)

You could use gst_audio_format_build_integer() here

@@ +963,3 @@
+  }
+
+  outcaps = gst_caps_new_simple ("audio/x-raw",

It's recommended to create audio caps from a GstAudioInfo

@@ +1013,3 @@
+  }
+
+  query = gst_query_new_allocation(outcaps, TRUE);

We don't necessarly need a buffer pool here, do we?

::: sys/dshowdecwrapper/gstdshowvideodec.cpp
@@ +54,2 @@
 #include "gstdshowvideodec.h"
+#include <gst/video/video.h>

Not sure where the video decoder creates the srcpad caps. It should use a GstVideoInfo for creating the caps though

@@ +143,3 @@
+   "framerate=" GST_VIDEO_FPS_RANGE "," \
+   "width=" GST_VIDEO_SIZE_RANGE "," \
+   "height=" GST_VIDEO_SIZE_RANGE

GST_VIDEO_CAPS_MAKE("YUY2")

@@ +332,3 @@
     GST_BUFFER_DURATION (buf) = clip_stop - clip_start;
 
+    gst_buffer_map(buf, &map, GST_MAP_WRITE);

Can also use gst_buffer_fill() here but I guess map() and memcpy() is simpler for this case

@@ +895,3 @@
           GST_TIME_ARGS (vdec->segment->stop));
 
+      /*HELP: update flag from segment event now gone, is flushing on segment needed?? */

No

@@ +964,3 @@
+      map.data, GST_BUFFER_TIMESTAMP (buffer), stop,
+      map.size, discont);
+  gst_buffer_unmap(buffer, &map);

Same question as for the audio decoder here

Thanks for the review :)

> Ideally you would also port them to GstVideoDecoder and GstAudioDecoder ;)

Yes, thought I would get a straightforward port working first and possibly look at converting later.


> +  gst_dshowaudiodec_parent_class = (GstElementClass *)
> g_type_class_peek_parent (klass);
> 
> G_DEFINE_TYPE() does that for you already

If I remove that line then parent_class is set to null and it crashes...
I looked a while and am not sure what is happening. These classes are being registered differently than most, with qdata and a base_init.
 

> That would've been broken before already, but is it guaranteed that
> PushBuffer() creates a copy of the memory if it needs it after the function has
> returned?

PushBuffer copies the data directly into a DirectShow buffer, so I think all is good here.


> +  query = gst_query_new_allocation(outcaps, TRUE);
> 
> We don't necessarly need a buffer pool here, do we?
> 

I originally didn't have it, but I tried adding one to the video decoder and noticed lower CPU utilization so I figured I would do it for both. Would a pool not be more efficient than allocating a fresh buffer each time?

Created attachment 292072 [details] [review]
DirectShow decoder update to 1.x patch

Patch updated per review

Review of attachment 292072 [details] [review]:

Thanks for updating the patch :)

::: sys/dshowdecwrapper/gstdshowaudiodec.cpp
@@ +211,3 @@
     GST_BUFFER_DURATION (out_buf) = duration;
+
+    gst_buffer_set_size(out_buf, gst_buffer_fill(out_buf, 0, pBuffer, size));

Please do this in two separate lines, and also check if gst_buffer_fill() was successful

::: sys/dshowdecwrapper/gstdshowvideodec.cpp
@@ +789,1 @@
+  caps_out = gst_video_info_to_caps(&video_info);

This code is a bit weird. Maybe don't use GstVideoInfo here and really just do what the old code was doing. Exactly what the old code was doing.

Created attachment 292142 [details] [review]
DirectShow decoder update to 1.x patch

Thanks, updated!

Review of attachment 292142 [details] [review]:

Perfect, thanks for updating :)

::: sys/dshowdecwrapper/gstdshowaudiodec.cpp
@@ +211,3 @@
     GST_BUFFER_DURATION (out_buf) = duration;
+
+    if (gst_buffer_fill(out_buf, 0, pBuffer, size) != size) {

Now only this seems a bit problematic. As you see, getting a buffer from the pool does not allow you to set the size. I assume this will always be fine currently because you set the size of the pool to GetBufferSize() when you create a pool yourself (and downstream will usually never provide one for audio).

But if you ever get a buffer of a different size here, or if downstream provides a buffer pool to you, this is probably going to break in one way or another.


I would recommend for now to disable all the buffer pool magic as for audio we don't use them yet... exactly for the reason mentioned above. No downstream will currently provide you a buffer pool with audio either for that reason.

So better comment out the allocation query stuff below, and just do a gst_buffer_new_allocate() above with the correct size.

@@ +979,3 @@
+    gst_query_parse_nth_allocation_pool (query, 0, &pool, &pool_size, &pool_min, 
+      &pool_max);
+  }

You should also handle the allocators from downstream here and set them on the pool... if any.

If you get an allocator and don't want to use a pool, you could btw use gst_buffer_new_allocate() with that allocator. That is something that will more likely happen for audio, and only that case is currently handled in the GstAudioDecoder base class (that is: downstream pools are ignored, but if there is an allocator we're going to use it).

> But if you ever get a buffer of a different size here, or if downstream
> provides a buffer pool to you, this is probably going to break in one way or
> another.

Makes sense. I'll just remove the pool code altogether for audio then, especially since the eventual goal is to port to GstAudioDecoder anyway.

Created attachment 292151 [details] [review]
DirectShow decoder update to 1.x patch

Removed audio buffer pool.

Thanks, I merged this but removed the now unused "query" variable :)

commit 54a7bdc0be3a4e3c7500a94d8eb038f6d6d641a7
Author: Matthew Bader <matt@hyperlobic.net>
Date:   Thu Dec 4 15:52:44 2014 -0500

    dshowdecwrapper: Port to 1.x
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740945