[Bug 740945] Patch to port DirectShow decoder plugin to 1.x

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=740945
  GStreamer | gst-plugins-bad | 1.x

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #291855|none                        |needs-work
             status|                            |

--- Comment #2 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-12-01 09:04:54 UTC ---
Review of attachment 291855:
 --> (https://bugzilla.gnome.org/review?bug=740945&attachment=291855)

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

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.