[Bug 705129] androidmedia: add support for video encoding

Sun Aug 25 00:41:16 PDT 2013

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

--- Comment #6 from cee1 <fykcee1 at gmail.com> 2013-08-25 07:41:10 UTC ---
(In reply to comment #4)
> Review of attachment 250440 [details]:
> 
> ::: 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 :(
I don't known any way to figure out the stride.
BTW, can stride of a format be equal to width of the format? I mean the stride
is something only to do with the underling hardware?

> @@ +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
https://github.com/cee1/gst-plugins-bad/commit/e18c12ff470723140cd0dd9d8579008997f227a9

> @@ +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
Do you mean encoder->buffer_size = info->size ?

buffer_size is calculated with a stride value which has not been set in
GstVideoInfo, so the value of info.size is not equal to buffer_size's ?

> @@ +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.
https://github.com/cee1/gst-plugins-bad/commit/9910ec07a194f46bcc1d0f2a49739f09e4b3c656

> @@ +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
I'll try to do this when all other parts are OK.

> @@ +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
https://github.com/cee1/gst-plugins-bad/commit/69f218d9d2839f5458b40016f9ee44853af71543

> @@ +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?
I don't test it, but this is copied from
http://developer.android.com/reference/android/media/MediaFormat.html#KEY_I_FRAME_INTERVAL

> 
> @@ +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
For decoder, I find some code taking care of the too small MediaCodec buffers:
http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/androidmedia/gstamcvideodec.c?id=2e8af6973fb2cf9c239305d56b3a290fcaa93216#n1524
Will it also be removed for decoder?

Nevertheless, my changeset is here:
https://github.com/cee1/gst-plugins-bad/commit/ce4b83d4613e42284c0ec051f8da6a0843935c4d

> @@ +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()
https://github.com/cee1/gst-plugins-bad/commit/5f8f16d56901c1925879f30795818dfd09dc8b67

> @@ +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
I have no idea.
For gst-omx, it will get the codec_data from omx_buf of
OMX_BUFFERFLAG_CODECCONFIG type.
For h264, it is in stream.
For other format which may have codec_data in GstAmcFormat, but no 
INFO_OUTPUT_FORMAT_CHANGED returned for the first time... 

> @@ +1521,3 @@
> +  self->flushing = TRUE;
> +
> +  if (self->first_set_format) {
> 
> Can not happen in start
https://github.com/cee1/gst-plugins-bad/commit/39a8c8467da7006650d684d52a204b51a3e8045f

> @@ +1725,3 @@
> +
> +static gboolean
> +gst_amc_video_enc_reset (GstVideoEncoder * encoder, gboolean hard)
> 
> The reset vfunc is deprecated in git master, use flush instead
https://github.com/cee1/gst-plugins-bad/commit/1d243f87e83a424bbde9c1982afa9070318683a8

> ::: 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
https://github.com/cee1/gst-plugins-bad/commit/c975cc4f9dfb2f2250b49e024c09fc2ca5df4407

Sorry for not test these new patches yet (I don't have an android pad at hand).
I'll try to find a one and test the new patches next week.

-- 
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.