[Bug 705129] androidmedia: add support for video encoding

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=705129
  GStreamer | gst-plugins-bad | git

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

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

--- Comment #4 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-08-19 10:31:20 UTC ---
Review of attachment 250440:
 --> (https://bugzilla.gnome.org/review?bug=705129&attachment=250440)

::: sys/androidmedia/gstamcvideoenc.c
@@ +518,3 @@
+  gst_amc_format_set_int (format, "bitrate", encoder->bitrate * 1024);
+  gst_amc_format_set_int (format, "color-format", color_format);
+  stride = GST_ROUND_UP_4 (info->width);        /* safe (?) */

No, not safe. This is (if anything) only correct for planar YUV formats :(

@@ +548,3 @@
+  if (encoder->codec_data)
+    gst_amc_format_set_buffer (format, "csd-0", encoder->codec_data,
+        encoder->codec_data_size);

There is no codec_data in the encoder on the input side

@@ +559,3 @@
+    case GST_VIDEO_FORMAT_I420:
+      encoder->buffer_size =
+          stride * slice_height + 2 * ((stride / 2) * (slice_height + 1) / 2);

info.size

@@ +564,3 @@
+    case GST_VIDEO_FORMAT_NV21:
+      encoder->buffer_size =
+          stride * slice_height + (stride * ((slice_height + 1) / 2));

info.size

@@ +677,3 @@
+  gst_caps_set_simple (caps, "width", G_TYPE_INT, width,
+      "height", G_TYPE_INT, height,
+      "framerate", G_TYPE_FLOAT, frame_rate, NULL);

framerate is always a fraction, there's gst_util_double_to_fraction() for
example.

@@ +720,3 @@
+  gst_element_class_add_pad_template (element_class, templ);
+
+  caps = create_src_caps (codec_info);

create_sink_caps() and create_src_caps() look like almost copies of the decoder
variants. Maybe refactor and put them into gstamc.c

@@ +825,3 @@
+
+  g_object_class_install_property (gobject_class, PROP_I_FRAME_INTERVAL,
+      g_param_spec_uint ("i-frame-int", "I-frame interval",

i-frame-interval

@@ +826,3 @@
+  g_object_class_install_property (gobject_class, PROP_I_FRAME_INTERVAL,
+      g_param_spec_uint ("i-frame-int", "I-frame interval",
+          "The frequency of I frames expressed in secs between I frames (0 for
automatic)",

secs->seconds

Is this really in seconds or in number of frames?

@@ +1082,3 @@
+  gboolean ret = FALSE;
+
+  need_tmp_buffer = buffer_info->size < self->buffer_size;

What's this tmp_buffer stuff? If the MediaCodec buffers are too small to hold a
complete frame, I would just error out. It would be a bug in the MediaCodec
implementation.
Please simplify this code accordingly

@@ +1243,3 @@
+
+    srcpad = GST_VIDEO_ENCODER_SRC_PAD (encoder);
+    out_buf = gst_buffer_new_allocate (NULL, buffer_info->size, NULL);

gst_video_encoder_allocate_output_frame() and
gst_video_encoder_allocate_output_buffer()

@@ +1246,3 @@
+    gst_buffer_fill (out_buf, 0, buf->data + buffer_info->offset,
+        buffer_info->size);
+

You probably also have to handle (differently) codec_data or header buffers
here. They might appear in the GstAmcFormat as csd-0, csd-1, etc fields. Or
maybe inside the stream. Depending on codec/stream-format they need to be put
in-stream or into the caps

@@ +1521,3 @@
+  self->flushing = TRUE;
+
+  if (self->first_set_format) {

Can not happen in start

@@ +1651,3 @@
+  g_free (self->codec_data);
+  self->codec_data = codec_data;
+  self->codec_data_size = codec_data_size;

There's no codec_data on the input side of the encoder

@@ +1725,3 @@
+
+static gboolean
+gst_amc_video_enc_reset (GstVideoEncoder * encoder, gboolean hard)

The reset vfunc is deprecated in git master, use flush instead

@@ +1799,3 @@
+  duration = frame->duration;
+
+  while (offset < gst_buffer_get_size (frame->input_buffer)) {

There should always be enough space to put one raw frame into one MediaCodec
input buffer. If not, return GST_FLOW_ERROR :)

::: sys/androidmedia/gstamcvideoenc.h
@@ +1,3 @@
+/*
+ * Copyright (C) 2012, Collabora Ltd.
+ *   Author: Sebastian Dröge <sebastian.droege at collabora.co.uk>

You probably want to add your own copyright 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.