[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

--- Comment #7 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-08-26 07:55:28 UTC ---
(In reply to comment #6)
> (In reply to comment #4)
> > Review of attachment 250440 [details] [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?

Setting "stride" on the MediaFormat does not help I guess? The stride mostly
depends on the underlying hardware, yes. And in theory it can be different for
each plane even.

> > @@ +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 ?

Yes, my mistake. Keep the code as is :)

> > @@ +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

So it is seconds indeed... ok :)

> > 
> > @@ +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?

No, for a decoder it is perfectly valid for an encoded frame to not fit in a
single buffer.

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

Looks good :)

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

Is it actually returned in the output format if you get it later? If that is
the case I think it is safe to assume that the output format is final once you
get the first output buffer.

For in-stream codec_data you probably get a flag set on the buffer too, like in
gst-omx.

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