[Bug 755510] vpx: create base class for vpx encoders and decoders

[GStreamer (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> changed:

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

--- Comment #3 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
Review of attachment 312006:
 --> (https://bugzilla.gnome.org/review?bug=755510&attachment=312006)

Some comment, will need to be rebased first. Let me know if you have time,
otherwise I'll take care.

::: ext/vpx/gstvp8dec.c
@@ +129,3 @@
 {
+  gst_tag_list_add (list, GST_TAG_MERGE_REPLACE,
+      GST_TAG_VIDEO_CODEC, "VP8 video", NULL);

Could also be a static const char on the class structure.

@@ +136,2 @@
 {
+  return &vpx_codec_vp8_dx_algo;

We should use class attribute (members on the class structure) for static
stuff.

@@ +143,3 @@
 {
+  /*nothing to do here as stream info set by call peek_stream_info is correct
for vp8 */
+  return;

Can't we just not implement virtual functions we don't need ?

@@ +157,2 @@
   gst_video_decoder_negotiate (GST_VIDEO_DECODER (dec));
+  vpxclass->send_tags (dec);

I have the impression there is no reason to have to call this in the subclass.

@@ +160,3 @@

+static gboolean
+gst_vp8_dec_get_valid_format (GstVPXDec * dec, vpx_image_t * img,

I'd try to find a better name for this one.

@@ +174,3 @@
+static void
+gst_vp8_dec_handle_resolution_change (GstVPXDec * dec, vpx_image_t * img,
+    GstVideoFormat fmt)

I would not find (except for the hardcoded I420) any reason why this isn't
generic. Any idea ?

::: ext/vpx/gstvp9dec.c
@@ +152,3 @@
+   * As this element supports multiple formats, format will be negotiated
dynamically while handling frame
+   */
+  return;

It should be static, or at least no have to implement.

@@ +203,2 @@
+    if (send_tags)
+      vpxclass->send_tags (dec);

It's suspicious that this differ.

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