GNOME Bugzilla – Bug 705129
androidmedia: add support for video encoding
Last modified: 2014-05-26 14:29:41 UTC
Created attachment 250440 [details] [review] The patch which adds video encoding support though android media codec Original report: https://bugs.freedesktop.org/show_bug.cgi?id=57831
Created attachment 250441 [details] [review] A related patch removes g_assert_not_reached when encounter "unknown color formats"
Quoting from a mail that cee1 sent me: > Sorry to ask the obvious... but does it work? :) Are there any known > problems left? Yes, at least for the Huawei MediaPad 10 FHD at my hand. My testing pipeline: * On the pad: "videotestsrc is-live=true do-timestamp=true name=videosrc ! capsfilter caps=\"video/x-raw,width=1024,height=768,framerate=8/1\" ! amcvidenc-omxk3videoencoderavc bitrate=1024 i-frame-int=8 ! rtph264pay ! queue ! udpsink host=192.168.0.102 port=3057" * On the PC side: "udpsrc uri=udp://0.0.0.0:3057 caps=\"application/x-rtp,media=(string)video,clock-rate=(int)90000,encoding-name=(string)H264,payload=(int)96\" ! rtpjitterbuffer latency=10 ! rtph264depay ! avdec_h264 ! videoscale ! autovideosink" Known problems left: * Setting profile/level in GstAmcFormat will render a codec configuration failure. * I don't know any way to figure out stride. * The branches handle "width != stride" are not tested. On my testing pad, it will always render a configuration failure if width is not a multiplier of 4. Even for width that is a multiplier of 4, the codec may not create a correct encoded stream. For widths those are multiplier of 16, I tried some, all of them work fine. * On my testing pad, the PC side will not decode correctly if not start first. I guess this is because the codec generates SPS/PPS only once. I tried h264parse config-interval=1, it fixed the problem partially -- a bad picture appears on the PC side periodically, which is not happened for "no h264parse + PC side starts first" case.
Review of attachment 250441 [details] [review]: ::: sys/androidmedia/gstamc.c @@ +1865,3 @@ GST_ERROR ("Codec has unknown color formats, ignoring"); valid_codec = FALSE; + // g_assert_not_reached (); No C99 comments please. But you're right, there should be no g_assert_not_reached() here, it should be removed :)
Review of attachment 250440 [details] [review]: ::: 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@collabora.co.uk> You probably want to add your own copyright here
Created attachment 252195 [details] [review] A related patch removes g_assert_not_reached when encounter "unknown color formats" C99 compatible
(In reply to comment #4) > Review of attachment 250440 [details] [review]: > > ::: 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@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.
(In reply to comment #6) > (In reply to comment #4) > > Review of attachment 250440 [details] [review] [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.
I've tested these new patches, they work on my pad. I notice an API change, the patch for it is here: https://github.com/cee1/gst-plugins-bad/commit/6bcce47d6a3459e14dba3ce2a879931055ad7903 (In reply to comment #7) > > > @@ +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. But that flag in gst-omx is set by underling OpenMAX component, right? In gstamcvideoenc when to set such a flag? I tried to use gst_amc_codec_get_output_format()(and hope there is csd-0 or csd-1)[1] but got a SEGV[2]. So, it seems I can only retrieve in-stream codec_data, and I need to parse buffers for each codec which has in-stream codec_data -- then, why not append a *parse(h264parse, etc) after gstamcvideoenc? ---- [1] diff --git a/sys/androidmedia/gstamcvideoenc.c b/sys/androidmedia/gstamcvideoenc.c index 79b7f32..0abef91 100644 --- a/sys/androidmedia/gstamcvideoenc.c +++ b/sys/androidmedia/gstamcvideoenc.c @@ -1276,18 +1276,26 @@ retry: GST_DEBUG_OBJECT (self, "Output format has changed"); +#if 0 format = (idx == INFO_OUTPUT_FORMAT_CHANGED) ? gst_amc_codec_get_output_format (self->codec) : self->first_set_format; +#else + format = gst_amc_codec_get_output_format (self->codec); +#endif [2] SIGSEGV: I/DEBUG ( 1708): backtrace: I/DEBUG ( 1708): #00 pc 00007e90 /system/lib/libstagefright_foundation.so (android::AMessage::writeToParcel(android::Parcel*) const+211) I/DEBUG ( 1708): #01 pc 00015793 /system/lib/libmedia_jni.so (android::ConvertMessageToMap(_JNIEnv*, android::sp<android::AMessage> const&, _jobject**)+886) I/DEBUG ( 1708): #02 pc 0000f199 /system/lib/libmedia_jni.so (android::JMediaCodec::getOutputFormat(_JNIEnv*, _jobject**) const+32) I/DEBUG ( 1708): #03 pc 0000f637 /system/lib/libmedia_jni.so I/DEBUG ( 1708): #04 pc 0001df30 /system/lib/libdvm.so (dvmPlatformInvoke+112) I/DEBUG ( 1708): #05 pc 0004d183 /system/lib/libdvm.so (dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*)+394) I/DEBUG ( 1708): #06 pc 00027360 /system/lib/libdvm.so I/DEBUG ( 1708): #07 pc 0002bc68 /system/lib/libdvm.so (dvmInterpret(Thread*, Method const*, JValue*)+180) I/DEBUG ( 1708): #08 pc 0005f8f5 /system/lib/libdvm.so (dvmCallMethodV(Thread*, Method const*, Object*, bool, JValue*, std::__va_list)+272) I/DEBUG ( 1708): #09 pc 0004c983 /system/lib/libdvm.so I/DEBUG ( 1708): #10 pc 001fcfeb /data/data/com.lemote.mcplayer/lib/libgstreamer_android.so (gst_amc_codec_get_output_format+38)
(In reply to comment #7) > > > 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. Here is the patch: https://github.com/cee1/gst-plugins-bad/commit/fd81d227da591db89c320317cd4cba587695fc94 Notes: 1. MPEG4 encoding is not tested, since the pad at hand does not support it. 2. For H264 encoding, the message "got codecconfig in byte-stream format" is printed. And it seems android will not return INFO_OUTPUT_FORMAT_CHANGED for an encoder (see https://android.googlesource.com/platform/cts/+/android-4.3_r3.1/tests/tests/media/src/android/media/cts/EncodeDecodeTest.java, line 539), I have to maintain the setting format -- the patch is here: https://github.com/cee1/gst-plugins-bad/commit/1f683738886b4447c68bd325035cad07dd28cd2f (In reply to comment #6) > (In reply to comment #4) > > Review of attachment 250440 [details] [review] [details]: > > @@ +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. Here is the patch: https://github.com/cee1/gst-plugins-bad/commit/ae0b7a92d5a6f0ac21a7cc736eb56f6dce5ccffd Note: 1. The audio/video decoding is not tested. 2. Not merge the logic of "Don't put the level restrictions on the sinkpad caps" * Shall we remove "level" restrictions from both decoder and encoder?
Created attachment 256807 [details] [review] androidmedia: add video encoding support V2, changes since V1: 1. "There's no codec_data for input side of the encoder" [https://github.com/cee1/gst-plugins-bad/commit/6414fd3e1f02e3c35f5268502ba8fa5f4ebcf1c8] 2. "framerate in caps is always a fraction" [https://github.com/cee1/gst-plugins-bad/commit/590acdfb4240cb306af120f20c84e11831c6854e] 3. "Rename property 'i-frame-int' to 'i-frame-interval'" [https://github.com/cee1/gst-plugins-bad/commit/aa77b69172eef819a6ae4092e4050a48e3790583] 4. "The reset vfunc is deprecated in git master, use flush instead" [https://github.com/cee1/gst-plugins-bad/commit/6e33abcb59bc1e63d234d98b567d26fcc59b917f] 5. "use gst_video_encoder_allocate_output_buffer instead of gst_buffer_new_allocate" [https://github.com/cee1/gst-plugins-bad/commit/cb8b7f52c2fe2c7513a6c3892c6cb713765c870a] 6. "code cleanup in gst_amc_video_enc_start" [https://github.com/cee1/gst-plugins-bad/commit/16f732b0ef1be0c7b6e95136fc4ebf37f9034ebb] 7. "update copyright" [https://github.com/cee1/gst-plugins-bad/commit/3ae9ef4cc25e6f4c82b29893776049cf604080ad] 8. "There should always be enough space to put one raw frame into one MediaCodec input buffer" [https://github.com/cee1/gst-plugins-bad/commit/dca7201487a94540afedbdd87c0f89c81c6505c2] 9. "Using gst_element_class_set_metadata." [https://github.com/cee1/gst-plugins-bad/commit/3ff97271f6aeb067a4ab0021ab30aa5da81ef616] 10. "gstamcvideoenc.c: handling codec_data" [https://github.com/cee1/gst-plugins-bad/commit/fd81d227da591db89c320317cd4cba587695fc94] 11. "INFO_OUTPUT_FORMAT_CHANGED is not expected for an encoder" [https://github.com/cee1/gst-plugins-bad/commit/1f683738886b4447c68bd325035cad07dd28cd2f] Tested it with the following pipelines: [*] videotestsrc is-live=true do-timestamp=true ! capsfilter caps="video/x-raw,width=1024,height=768,framerate=8/1" ! amcvidenc-omxk3videoencoderavc bitrate=1024 i-frame-interval=8 ! rtph264pay ! queue ! udpsink host=192.168.1.104 port=3057 [*] videotestsrc is-live=true do-timestamp=true ! capsfilter caps="video/x-raw,width=1024,height=768,framerate=8/1" ! amcvidenc-omxk3videoencoderavc bitrate=1024 i-frame-interval=8 ! queue ! capssetter caps="video/x-h264,profile=baseline" ! amcviddec-omxk3videodecoderavc ! eglglessink *** Note *** 1. Replace the encoder amcvidenc-omxk3videoencoderavc with the actual encoder on your pad. 2. Replace the decoder amcviddec-omxk3videodecoderavc with the actual decoder on your pad. 3. Replace IP "192.168.1.104" with the receiver's IP
Created attachment 256808 [details] [review] Move shared codes (i.e. create_src|sink_caps) to gstamc.c This patch moves some shared codes(create_src|sink_caps) from gstamcvideodec, gstamcvideoenc and gstamcaudiodec to gstamc.c This patch merges commit 2b94641 "amcvideodec: Don't put the level restrictions on the sinkpad caps"
Three more patches for making amcvideoenc work better: 1. androidmedia: new prototype for color_format/video_format conversion (https://github.com/cee1/gst-plugins-bad/commit/98c994f953dd8faa658122fbabbcf034a367b28d) 2. androidmedia: add gst_amc_color_format_copy (https://github.com/cee1/gst-plugins-bad/commit/ff77dcbb01a7af0a2a3c1d940abc84b2721afedd) 3.androidmedia: add a COLOR_FormatYCbYCr to GST_VIDEO_FORMAT_YUY2 mapping. (https://github.com/cee1/gst-plugins-bad/commit/149ea45cc8d6c459f7dd59cd72eb59e7a237fe15) There are two concepts of color_format: the reported color_format and the "real" color_format == The reported color_format == That's the supporting color_format reported by the codec. Some considerations for it: a) codec reports supporting color_format CA, then it may need to map CA to gstvideoformat VA in amcvideodec/enc, and then map back. VA may be mapped to CA' in current code, e.g. COLOR_QCOM_FormatYUV420SemiPlanar -> GST_VIDEO_FORMAT_NV12 -> COLOR_FormatYUV420SemiPlanar Which causes amc_codec_configure failed with "not supported color format" b) codec may report wrong color_format CA, which is actually color_format CB, so we have to map CA to gstvideoformat VB and mapped back to CA. a) and b) are handled in patch 1: "androidmedia: new prototype for color_format/video_format conversion" == The "real" color_format == That's used in *_fill_buffer in amcvideodec, *_fill_buffer will copy a "sparse-layouted" frame of color_format CA to a "densely-layouted" frame of elated gstvideoformat VA. Patch 2: "androidmedia: add gst_amc_color_format_copy" introduces a GstAmcColorFormatInfo to describe a sparse layout, a new function gst_amc_color_format_copy to copy data from a "sparse-layouted" frame to a "densely-layouted" frame and reverse. For amcvideoenc, it needs to evaluate the size of a "sparse-layouted" frame(color_format_info.frame_size), which is used to determine whether go through the fast path in *_fill_buffer. The color_format_info.frame_size is calculated in gst_amc_color_format_info_set. Note, I'm not sure the frame_size calculations for COLOR_TI_FormatYUV420PackedSemiPlanar and COLOR_QCOM_FormatYUV420PackedSemiPlanar64x32Tile2m8ka are correct, please review. Overall, patch 2 moves lots of codes in *_fill_buffer to gst_amc_color_format_copy, make the logic shared between amcvideodec and amcvideoenc. BTW, Patch 1 and patch 2 move all current hacks to gst_amc_color_format_to_video_format /gst_amc_video_format_to_color_format /gst_amc_color_format_info_set in gstamc.c A full list of the patches can be found in my github's master branch: https://github.com/cee1/gst-plugins-bad/commits/master
Hi, i does not really work for me! that's too bad i would really need it :/ There are still too much problems: the buffers sizes are wrong, timeouts, wrong caps ... I was not able to produce any video. I hope someone will be able to provide working encoders ... Regards.
Hi, As far as I known, someone has managed to use it. Could you try the following: 1. http://lists.freedesktop.org/archives/gstreamer-devel/2013-October/043675.html 2. http://lists.freedesktop.org/archives/gstreamer-devel/2013-October/043623.html And send me the logs. BTW, I think gstreamer-devel@lists.freedesktop.org is a better place to discuss this :)
Comment on attachment 252195 [details] [review] A related patch removes g_assert_not_reached when encounter "unknown color formats" This one was committed some time ago already
Review of attachment 256807 [details] [review]: ::: sys/androidmedia/gstamcvideoenc.c @@ +820,3 @@ + g_object_class_install_property (gobject_class, PROP_BIT_RATE, + g_param_spec_uint ("bitrate", "Bitrate", "Bitrate in kbit/sec", 1, + 100 * 1024, BIT_RATE_DEFAULT, Why limit it at that number? Just use G_MAXINT :) And we do bitrate in bit/s usually
Review of attachment 256808 [details] [review]: I think I would prefer to have the "hack" variants merged with the real ones. Just pass the codec name to all these functions
Comment on attachment 256807 [details] [review] androidmedia: add video encoding support I pushed this one now so we finally get it in... but please provide a follow-up patch for fixing the bitrate property :)
commit 22c0464aa6d67e8407bd745a110971296ce1fe2d Author: Chen Jie <chenj@lemote.com> Date: Mon Apr 1 07:49:01 2013 +0000 androidmedia: add support for video encoding https://bugzilla.gnome.org/show_bug.cgi?id=705129
(In reply to comment #18) > (From update of attachment 256807 [details] [review]) > I pushed this one now so we finally get it in... but please provide a follow-up > patch for fixing the bitrate property :) [PATCH] androidmedia:encoder unit of Bitrate is now bit/sec https://github.com/cee1/gst-plugins-bad/commit/da71c581507e87604ee90a98310fc1276db0c50c
Created attachment 277181 [details] [review] encoder: change unit of Bitrate to bit/sec
Created attachment 277188 [details] [review] androidmedia: move create_src|sink_caps to gstamc.c
Created attachment 277191 [details] [review] androidmedia: add gst_amc_color_format_copy
commit 8e0bbc9e32aa99da981eb7396f491d0f4aef91d6 Author: Chen Jie <chenj@lemote.com> Date: Wed Nov 13 18:41:33 2013 +0800 androidmedia: add gst_amc_color_format_copy gst_amc_color_format_copy will copy in/out a frame resides at a GstAmcBuffer. Lots of codes in gst_amc_video_*_fill_buffer are moved to this new function. commit 557d6b974fe98d09ddd0a500548b8a13211dab1c Author: Chen Jie <chenj@lemote.com> Date: Tue Oct 8 21:11:48 2013 +0800 androidmedia: move create_src|sink_caps to gstamc.c Some hack logic needs also to be present in create_src|sink_caps, for working around some broken codecs. These hacks are hidden in color_format/video_format conversion -- the prototypes of these functions are also changed to include more args for hack judgement. Also in case of multi-color_formats mapped to one video_format, then map that video_format back will not give the original color_format, which causes gst_amc_codec_configure failed with something like 'does not support color format N'. The new prototype involves with GstAmcCodecInfo and mime, which ensures the converted color_format is supported by the codec. A COLOR_FormatYCbYCr to GST_VIDEO_FORMAT_YUY2 mapping is also added, in order to work around bugs in OMX.k3.video.decoder.avc(which incorrectly reports supporting COLOR_FormatYCbYCr, which is actually COLOR_FormatYUV420SemiPlanar). There are already hacks for this in gst_amc_video_format_to_color_format, gst_amc_color_format_to_video_format and gst_amc_color_format_info_set, but the codec will still not work(be ignored because of "has unknown color formats") without adding this mapping.