Menu
▾
▴
gstreamer-openmax — Development list for GStreamer OpenMax plugins
You can subscribe to this list here.
| 2007 |
Jan
|
Feb
|
Mar
|
Apr
|
May
|
Jun
|
Jul
(1) |
Aug
(2) |
Sep
|
Oct
(1) |
Nov
|
Dec
(8) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 2008 |
Jan
(5) |
Feb
(3) |
Mar
|
Apr
(2) |
May
(4) |
Jun
(1) |
Jul
(11) |
Aug
(31) |
Sep
(2) |
Oct
(21) |
Nov
(16) |
Dec
(56) |
| 2009 |
Jan
(12) |
Feb
(5) |
Mar
(34) |
Apr
(9) |
May
(5) |
Jun
(7) |
Jul
(18) |
Aug
(5) |
Sep
(2) |
Oct
(6) |
Nov
(50) |
Dec
|
| 2010 |
Jan
(3) |
Feb
(67) |
Mar
(135) |
Apr
(30) |
May
(2) |
Jun
(11) |
Jul
|
Aug
(18) |
Sep
(12) |
Oct
(4) |
Nov
(17) |
Dec
(11) |
| 2011 |
Jan
(14) |
Feb
(2) |
Mar
|
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
|
|
From: Rob C. <ro...@ti...> - 2010-03-29 13:06:12
|
This way omx_handle is always valid, and can be used in get/set_property
methods.
---
v2: add gstomx_core_new() which calls g_omx_core_new(),
gstomx_get_component_info(), and then g_omx_core_init() to avoid a
dependencies on gstomx from g_omx
omx/gstomx.c | 13 +++++++++++++
omx/gstomx.h | 2 ++
omx/gstomx_base_filter.c | 8 +-------
omx/gstomx_base_sink.c | 5 +----
omx/gstomx_base_src.c | 5 +----
omx/gstomx_util.c | 8 ++++++--
omx/gstomx_util.h | 1 -
7 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/omx/gstomx.c b/omx/gstomx.c
index ff232b0..2a3a0b7 100644
--- a/omx/gstomx.c
+++ b/omx/gstomx.c
@@ -333,6 +333,19 @@ gstomx_get_component_info (void *core,
return TRUE;
}
+/* for now, to keep gst out of g_omx.. if later g_omx becomes gstomx
+ * then this could be combined with the existing g_omx_core_new()
+ * and g_omx_core_init() could be made static/private
+ */
+void *
+gstomx_core_new (void *object, GType type)
+{
+ GOmxCore *core = g_omx_core_new (object);
+ gstomx_get_component_info (core, type);
+ g_omx_core_init (core);
+ return core;
+}
+
GST_PLUGIN_DEFINE (GST_VERSION_MAJOR,
GST_VERSION_MINOR,
"omx",
diff --git a/omx/gstomx.h b/omx/gstomx.h
index b62773c..4f07fd6 100644
--- a/omx/gstomx.h
+++ b/omx/gstomx.h
@@ -33,6 +33,8 @@ GST_DEBUG_CATEGORY_EXTERN (gstomx_util_debug);
gboolean gstomx_get_component_info (void *core,
GType type);
+void * gstomx_core_new (void *object, GType type);
+
G_END_DECLS
#endif /* GSTOMX_H */
diff --git a/omx/gstomx_base_filter.c b/omx/gstomx_base_filter.c
index e12fcd2..019b52b 100644
--- a/omx/gstomx_base_filter.c
+++ b/omx/gstomx_base_filter.c
@@ -101,7 +101,6 @@ change_state (GstElement *element,
switch (transition)
{
case GST_STATE_CHANGE_NULL_TO_READY:
- g_omx_core_init (core);
if (core->omx_state != OMX_StateLoaded)
{
ret = GST_STATE_CHANGE_FAILURE;
@@ -141,10 +140,6 @@ change_state (GstElement *element,
}
break;
- case GST_STATE_CHANGE_READY_TO_NULL:
- g_omx_core_deinit (core);
- break;
-
default:
break;
}
@@ -875,8 +870,7 @@ type_instance_init (GTypeInstance *instance,
self->use_timestamps = TRUE;
- self->gomx = g_omx_core_new (self);
- gstomx_get_component_info (self->gomx, G_TYPE_FROM_CLASS (g_class));
+ self->gomx = gstomx_core_new (self, G_TYPE_FROM_CLASS (g_class));
self->in_port = g_omx_core_new_port (self->gomx, 0);
self->out_port = g_omx_core_new_port (self->gomx, 1);
diff --git a/omx/gstomx_base_sink.c b/omx/gstomx_base_sink.c
index b6a8955..f3e116f 100644
--- a/omx/gstomx_base_sink.c
+++ b/omx/gstomx_base_sink.c
@@ -381,8 +381,6 @@ activate_push (GstPad *pad,
static inline gboolean
omx_init (GstOmxBaseSink *self)
{
- g_omx_core_init (self->gomx);
-
if (self->gomx->omx_error)
return FALSE;
@@ -421,8 +419,7 @@ type_instance_init (GTypeInstance *instance,
GST_LOG_OBJECT (self, "begin");
- self->gomx = g_omx_core_new (self);
- gstomx_get_component_info (self->gomx, G_TYPE_FROM_CLASS (g_class));
+ self->gomx = gstomx_core_new (self, G_TYPE_FROM_CLASS (g_class));
self->in_port = g_omx_core_new_port (self->gomx, 0);
{
diff --git a/omx/gstomx_base_src.c b/omx/gstomx_base_src.c
index fea7631..49ddd54 100644
--- a/omx/gstomx_base_src.c
+++ b/omx/gstomx_base_src.c
@@ -54,7 +54,6 @@ start (GstBaseSrc *gst_base)
GST_LOG_OBJECT (self, "begin");
- g_omx_core_init (self->gomx);
if (self->gomx->omx_error)
return GST_STATE_CHANGE_FAILURE;
@@ -74,7 +73,6 @@ stop (GstBaseSrc *gst_base)
g_omx_core_stop (self->gomx);
g_omx_core_unload (self->gomx);
- g_omx_core_deinit (self->gomx);
if (self->gomx->omx_error)
return GST_STATE_CHANGE_FAILURE;
@@ -406,8 +404,7 @@ type_instance_init (GTypeInstance *instance,
GST_LOG_OBJECT (self, "begin");
- self->gomx = g_omx_core_new (self);
- gstomx_get_component_info (self->gomx, G_TYPE_FROM_CLASS (g_class));
+ self->gomx = gstomx_core_new (self, G_TYPE_FROM_CLASS (g_class));
self->out_port = g_omx_core_new_port (self->gomx, 1);
GST_LOG_OBJECT (self, "end");
diff --git a/omx/gstomx_util.c b/omx/gstomx_util.c
index b1c869a..59e6487 100644
--- a/omx/gstomx_util.c
+++ b/omx/gstomx_util.c
@@ -79,6 +79,8 @@ omx_error_to_str (OMX_ERRORTYPE omx_error);
static inline GOmxPort * get_port (GOmxCore *core, guint index);
+static void core_deinit (GOmxCore *core);
+
static inline void
port_free_buffers (GOmxPort *port);
@@ -288,6 +290,8 @@ g_omx_core_new (void *object)
void
g_omx_core_free (GOmxCore *core)
{
+ core_deinit (core);
+
g_sem_free (core->port_sem);
g_sem_free (core->flush_sem);
g_sem_free (core->done_sem);
@@ -316,8 +320,8 @@ g_omx_core_init (GOmxCore *core)
core->omx_state = OMX_StateLoaded;
}
-void
-g_omx_core_deinit (GOmxCore *core)
+static void
+core_deinit (GOmxCore *core)
{
if (!core->imp)
return;
diff --git a/omx/gstomx_util.h b/omx/gstomx_util.h
index 456f9a8..5c928ed 100644
--- a/omx/gstomx_util.h
+++ b/omx/gstomx_util.h
@@ -120,7 +120,6 @@ void g_omx_deinit (void);
GOmxCore *g_omx_core_new (void *object);
void g_omx_core_free (GOmxCore *core);
void g_omx_core_init (GOmxCore *core);
-void g_omx_core_deinit (GOmxCore *core);
void g_omx_core_prepare (GOmxCore *core);
void g_omx_core_start (GOmxCore *core);
void g_omx_core_pause (GOmxCore *core);
--
1.6.6
|
|
From: Felipe C. <fel...@gm...> - 2010-03-25 19:57:11
|
On Thu, Mar 25, 2010 at 9:23 PM, Rob Clark <ro...@ti...> wrote: > yeah. And it would be interesting to see more other gst elements, like demuxers, utilizing pad_alloc().. then it would be interesting to implement a GstOmxBuffer and implement pad_alloc handler. It doesn't make sense for a demuxer to do pad_alloc(). Imagine an efficient scenario where the filesrc mmaps the entire clip, then demuxer, some way or the other starts parsing the data and finds the interesting frames, then what? There are only two options: 1) pad_alloc() and memcpy 2) create a sub-buffer and push it For obvious reasons demuxers prefer 2), many decoders can operate directly on the data, and others, such as the ones in gst-dsp can mmap the data and make the DSP algo work on the system memory. So no, I don't see any point in demuxers doing pad_alloc(). Cheers. -- Felipe Contreras |
|
From: Rob C. <ro...@ti...> - 2010-03-25 19:23:54
|
On Mar 25, 2010, at 12:53 PM, Felipe Contreras wrote: > On Thu, Mar 25, 2010 at 6:37 PM, Rob Clark <ro...@ti...> wrote: >> On Mar 25, 2010, at 5:15 AM, Felipe Contreras wrote: >>> There has been some progress regarding tunneling, however, it was >>> found that GStreamer's base classes are not going to work for this >>> feature. More work will be needed before this feature lands in master. >>> My personal opinion is that we should stop trying to use GStreamer's >>> base classes and implement the sources/sinks from scratch that fit >>> omx. However, I do not have time to play with this idea right now. >> >> note that not using GStreamer's base classes for src/sink elements also means not using GStreamer's A/V sync, etc. > > Not really. GStreamer's base classes achieve A/V sync with code, not > magic, and gst-omx base classes can do the same thing. I don't think > it would be _that_ difficult, but we would have to wait and see. no, not magic.. just a task that I've seen frequently underestimated ;-) > >> On the other hand, it seems that khronos is on the verge of approving a change to allow "non-pre-announced" buffers (essentially officially allowing IL client to do pBuffer pointer copies instead of memcpy's) which in most cases is just as good as tunneling without all the drawbacks. >> >> So IMO, most use-cases where you think tunneling is a good idea, it is probably not. > > Completely agree. Perhaps there are some very advanced use-cases where > tunneling might provide a marginal advantage, but I don't think > tunneling is worth the trouble at this point in time. Even now sharing > buffers a la GStreamer's pad_alloc() is far from being exploited > properly, so we should probably start with that. yeah. And it would be interesting to see more other gst elements, like demuxers, utilizing pad_alloc().. then it would be interesting to implement a GstOmxBuffer and implement pad_alloc handler. BR, -R > > That being said tunneling is by far the most requested (only?) feature > of gst-omx. > > Cheers. > > -- > Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-25 17:53:19
|
On Thu, Mar 25, 2010 at 6:37 PM, Rob Clark <ro...@ti...> wrote: > On Mar 25, 2010, at 5:15 AM, Felipe Contreras wrote: >> There has been some progress regarding tunneling, however, it was >> found that GStreamer's base classes are not going to work for this >> feature. More work will be needed before this feature lands in master. >> My personal opinion is that we should stop trying to use GStreamer's >> base classes and implement the sources/sinks from scratch that fit >> omx. However, I do not have time to play with this idea right now. > > note that not using GStreamer's base classes for src/sink elements also means not using GStreamer's A/V sync, etc. Not really. GStreamer's base classes achieve A/V sync with code, not magic, and gst-omx base classes can do the same thing. I don't think it would be _that_ difficult, but we would have to wait and see. > On the other hand, it seems that khronos is on the verge of approving a change to allow "non-pre-announced" buffers (essentially officially allowing IL client to do pBuffer pointer copies instead of memcpy's) which in most cases is just as good as tunneling without all the drawbacks. > > So IMO, most use-cases where you think tunneling is a good idea, it is probably not. Completely agree. Perhaps there are some very advanced use-cases where tunneling might provide a marginal advantage, but I don't think tunneling is worth the trouble at this point in time. Even now sharing buffers a la GStreamer's pad_alloc() is far from being exploited properly, so we should probably start with that. That being said tunneling is by far the most requested (only?) feature of gst-omx. Cheers. -- Felipe Contreras |
|
From: Rob C. <ro...@ti...> - 2010-03-25 16:37:13
|
On Mar 25, 2010, at 5:15 AM, Felipe Contreras wrote: > Hi, > > On Thu, Mar 25, 2010 at 6:12 AM, Sidharth R <sv...@ya...> wrote: >> As per the following link : >> http://github.com/felipec/gst-openmax >> >> there has not been any updation on the source code associated with tunneling after 27th April 2009. Is the tunneling branch merged with the master branch of gst-openmax? Is tunneling fully functional in gst-openmax? >> >> Kindly reply to the above queries regarding tunneling. > > There has been some progress regarding tunneling, however, it was > found that GStreamer's base classes are not going to work for this > feature. More work will be needed before this feature lands in master. > My personal opinion is that we should stop trying to use GStreamer's > base classes and implement the sources/sinks from scratch that fit > omx. However, I do not have time to play with this idea right now. note that not using GStreamer's base classes for src/sink elements also means not using GStreamer's A/V sync, etc. On the other hand, it seems that khronos is on the verge of approving a change to allow "non-pre-announced" buffers (essentially officially allowing IL client to do pBuffer pointer copies instead of memcpy's) which in most cases is just as good as tunneling without all the drawbacks. So IMO, most use-cases where you think tunneling is a good idea, it is probably not. (That said, transcoding and some other edge cases like that, perhaps tunneling could be a good idea.. but then you don't need to replace GStreamer base classes for that.) Just my $0.02 :-) BR, -R |
|
From: Felipe C. <fel...@gm...> - 2010-03-25 10:15:20
|
Hi, On Thu, Mar 25, 2010 at 6:12 AM, Sidharth R <sv...@ya...> wrote: > As per the following link : > http://github.com/felipec/gst-openmax > > there has not been any updation on the source code associated with tunneling after 27th April 2009. Is the tunneling branch merged with the master branch of gst-openmax? Is tunneling fully functional in gst-openmax? > > Kindly reply to the above queries regarding tunneling. There has been some progress regarding tunneling, however, it was found that GStreamer's base classes are not going to work for this feature. More work will be needed before this feature lands in master. My personal opinion is that we should stop trying to use GStreamer's base classes and implement the sources/sinks from scratch that fit omx. However, I do not have time to play with this idea right now. Cheers. -- Felipe Contreras |
|
From: Sidharth R <sv...@ya...> - 2010-03-25 04:12:19
|
Sir, As per the following link : http://github.com/felipec/gst-openmax there has not been any updation on the source code associated with tunneling after 27th April 2009. Is the tunneling branch merged with the master branch of gst-openmax? Is tunneling fully functional in gst-openmax? Kindly reply to the above queries regarding tunneling. Sidharth Varier |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:49:12
|
On Wed, Mar 24, 2010 at 12:36 AM, Clark, Rob <ro...@ti...> wrote: > On Mar 23, 2010, at 5:25 PM, Felipe Contreras wrote: >> I don't like this. It would be better to pass the GType directly. >> >> But also, it's messing up g_omx and gstomx_ functions... Can you move >> g_omx_core_new to gstomx.c and make it gstomx_core_new() or something >> like that? > > I could do that for now. > > But actually I would like to change the g_omx_ stuff to gstomx_.. I think there are some parts of logic, such as handling the share_buffer vs !share_buffer cases, which could be consolidated in GOmxPort (or GstOmxPort). Similar for handling of EOS. Yeah, I've been meaning to fix the share_buffer stuff like that, but I haven't really looked into it. I don't think there's a need to mix gst stuff though. > And for upstream caps negotiation to work properly, we need to do a pad_alloc() before SendCommand(SetState, Idle) or SendCommand(EnablePort).. which could be more easily done in one place in GOmxPort. Haven't really thought about that... I would have to see the patches. -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:45:59
|
On Thu, Mar 18, 2010 at 1:59 AM, Rob Clark <ro...@ti...> wrote: > Refactor some common functionality, in particular the settings_changed_cb, > into an abstract base class. I don't have much interest in the audio stuff, so I just quickly glanced over it. Looks ok. -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:44:03
|
On Thu, Mar 18, 2010 at 1:58 AM, Rob Clark <ro...@ti...> wrote:
> ---
> omx/gstomx_base_filter.c | 62 ++++++++++++++++++++++++++++++++++++++
> omx/gstomx_base_sink.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
> omx/gstomx_base_src.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 210 insertions(+), 0 deletions(-)
>
> diff --git a/omx/gstomx_base_filter.c b/omx/gstomx_base_filter.c
> index 98b40d5..76ec085 100644
> --- a/omx/gstomx_base_filter.c
> +++ b/omx/gstomx_base_filter.c
> @@ -28,6 +28,8 @@
> enum
> {
> ARG_USE_TIMESTAMPS = GSTOMX_NUM_COMMON_PROP,
> + ARG_NUM_INPUT_BUFFERS,
> + ARG_NUM_OUTPUT_BUFFERS,
> };
>
> static void init_interfaces (GType type);
> @@ -182,6 +184,36 @@ set_property (GObject *obj,
> case ARG_USE_TIMESTAMPS:
> self->use_timestamps = g_value_get_boolean (value);
> break;
> + case ARG_NUM_INPUT_BUFFERS:
> + case ARG_NUM_OUTPUT_BUFFERS:
> + if (G_LIKELY (self->gomx->omx_handle))
> + {
Too nested.
How about:
if (G_UNLIKELY(!omx_handle)) {
GST_WARNING_OBJECT(self, "no component");
break;
}
/* rest of the stuff */
This applies all over the place.
> + OMX_PARAM_PORTDEFINITIONTYPE param;
> + OMX_U32 nBufferCountActual = g_value_get_uint (value);
> + GOmxPort *port = (prop_id == ARG_NUM_INPUT_BUFFERS) ?
> + self->in_port : self->out_port;
> +
> + G_OMX_INIT_PARAM (param);
> +
> + param.nPortIndex = port->port_index;
> + OMX_GetParameter (self->gomx->omx_handle, OMX_IndexParamPortDefinition, ¶m);
> +
> + if (nBufferCountActual < param.nBufferCountMin)
> + {
> + GST_ERROR_OBJECT (self, "buffer count %lu is less than minimum %lu",
> + nBufferCountActual, param.nBufferCountMin);
> + return;
> + }
> +
> + param.nBufferCountActual = nBufferCountActual;
> +
> + OMX_SetParameter (self->gomx->omx_handle, OMX_IndexParamPortDefinition, ¶m);
> + }
> + else
> + {
> + GST_WARNING_OBJECT (self, "no component");
> + }
> + break;
--
Felipe Contreras
|
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:38:22
|
On Thu, Mar 18, 2010 at 1:58 AM, Rob Clark <ro...@ti...> wrote:
> @@ -337,7 +339,23 @@ core_init (GOmxCore *core)
> core->omx_handle, core->omx_error);
>
> if (!core->omx_error)
> + {
> core->omx_state = OMX_StateLoaded;
> +
> + if (core->component_role)
> + {
> + OMX_PARAM_COMPONENTROLETYPE param;
> +
> + GST_DEBUG_OBJECT (core->object, "setting component role: %s",
> + core->component_role);
> +
> + G_OMX_INIT_PARAM (param);
> +
> + strcpy((char*)param.cRole, core->component_role);
How about strncpy with OMX_MAX_STRINGNAME_SIZE instead?
Otherwise looks ok. Thanks.
--
Felipe Contreras
|
|
From: Clark, R. <ro...@ti...> - 2010-03-23 22:36:59
|
On Mar 23, 2010, at 5:25 PM, Felipe Contreras wrote: > On Thu, Mar 18, 2010 at 1:58 AM, Rob Clark <ro...@ti...> wrote: >> This way omx_handle is always valid, and can be used in get/set_property >> methods. >> --- > >> @@ -282,12 +285,22 @@ g_omx_core_new (void *object) >> >> core->omx_state = OMX_StateInvalid; >> >> + /* it is a bit awkward to have to pass the element's g_class to >> + * g_omx_core_new(), but since this is called from type_inistance_init >> + * function, we can't rely on G_OBJECT_TYPE().. >> + */ >> + gstomx_get_component_info (core, G_TYPE_FROM_CLASS (g_class)); > > I don't like this. It would be better to pass the GType directly. > > But also, it's messing up g_omx and gstomx_ functions... Can you move > g_omx_core_new to gstomx.c and make it gstomx_core_new() or something > like that? I could do that for now. But actually I would like to change the g_omx_ stuff to gstomx_.. I think there are some parts of logic, such as handling the share_buffer vs !share_buffer cases, which could be consolidated in GOmxPort (or GstOmxPort). Similar for handling of EOS. And for upstream caps negotiation to work properly, we need to do a pad_alloc() before SendCommand(SetState, Idle) or SendCommand(EnablePort).. which could be more easily done in one place in GOmxPort. This is probably the subject for another patch(set).. but I thought maybe now would be a good time to bring up the idea and see if anyone had any issue with it. BR, -R > > Cheers. > > -- > Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:35:39
|
On Thu, Mar 18, 2010 at 1:59 AM, Rob Clark <ro...@ti...> wrote: > --- > omx/gstomx_base_filter.c | 12 ++++++++++++ > omx/gstomx_util.c | 2 ++ > 2 files changed, 14 insertions(+), 0 deletions(-) ACK. -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:34:46
|
On Thu, Mar 18, 2010 at 1:58 AM, Rob Clark <ro...@ti...> wrote: > --- > omx/gstomx_util.c | 29 +++++++++++++++++++++++++++++ > 1 files changed, 29 insertions(+), 0 deletions(-) ACK. -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:33:15
|
On Thu, Mar 18, 2010 at 1:58 AM, Rob Clark <ro...@ti...> wrote: > This removes some common code from all base classes, and makes it easier to > add new common properties. > > For now (and the foreseeable future) all common properties are read-only, > but a gstomx_set_property_helper() could be added later if needed. A bit hacky for my taste, but I guess there's no better way to implement this. ACK. -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:31:42
|
On Thu, Mar 18, 2010 at 1:58 AM, Rob Clark <ro...@ti...> wrote: > --- > omx/gstomx_aacdec.c | 7 +------ > omx/gstomx_aacenc.c | 22 ++++------------------ > omx/gstomx_adpcmdec.c | 7 +------ > omx/gstomx_adpcmenc.c | 12 ++---------- > omx/gstomx_amrnbdec.c | 7 +------ > omx/gstomx_amrnbenc.c | 12 ++---------- > omx/gstomx_amrwbdec.c | 7 +------ > omx/gstomx_amrwbenc.c | 12 ++---------- > omx/gstomx_audiosink.c | 7 +------ > omx/gstomx_base_filter.c | 2 +- > omx/gstomx_base_sink.c | 2 +- > omx/gstomx_base_src.c | 2 +- > omx/gstomx_base_videodec.c | 18 +++--------------- > omx/gstomx_base_videoenc.c | 13 ++++--------- > omx/gstomx_g711dec.c | 7 ++----- > omx/gstomx_g711enc.c | 7 ++----- > omx/gstomx_g729enc.c | 7 +------ > omx/gstomx_h263enc.c | 7 +------ > omx/gstomx_h264enc.c | 7 +------ > omx/gstomx_jpegenc.c | 20 ++++---------------- > omx/gstomx_mp2dec.c | 7 +------ > omx/gstomx_mp3dec.c | 7 +------ > omx/gstomx_mpeg4enc.c | 7 +------ > omx/gstomx_util.c | 6 +----- > omx/gstomx_util.h | 9 +++++++++ > omx/gstomx_videosink.c | 17 ++++------------- > omx/gstomx_volume.c | 7 +------ > omx/gstomx_vorbisdec.c | 7 +------ > 28 files changed, 55 insertions(+), 197 deletions(-) Looks fine to me. ACK. -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:25:10
|
On Thu, Mar 18, 2010 at 1:58 AM, Rob Clark <ro...@ti...> wrote: > This way omx_handle is always valid, and can be used in get/set_property > methods. > --- > @@ -282,12 +285,22 @@ g_omx_core_new (void *object) > > core->omx_state = OMX_StateInvalid; > > + /* it is a bit awkward to have to pass the element's g_class to > + * g_omx_core_new(), but since this is called from type_inistance_init > + * function, we can't rely on G_OBJECT_TYPE().. > + */ > + gstomx_get_component_info (core, G_TYPE_FROM_CLASS (g_class)); I don't like this. It would be better to pass the GType directly. But also, it's messing up g_omx and gstomx_ functions... Can you move g_omx_core_new to gstomx.c and make it gstomx_core_new() or something like that? Cheers. -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:17:30
|
On Thu, Mar 18, 2010 at 12:50 AM, Rob Clark <ro...@ti...> wrote:
> These apply on top of the last patchset ('rebased boilerplate reduction
> macros')
>
> Rob Clark (3):
> Construct GOmxPort objects in element constructor
> Simplify g_omx_port_setup()
> Don't hard-code port indexes
I'll apply these ASAP. But maybe I'll delay the last one. Thanks.
--
Felipe Contreras
|
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:16:45
|
On Wed, Mar 24, 2010 at 12:14 AM, Clark, Rob <ro...@ti...> wrote:
>
> On Mar 23, 2010, at 5:04 PM, Felipe Contreras wrote:
>
>> On Tue, Mar 16, 2010 at 12:29 AM, Rob Clark <ro...@ti...> wrote:
>>> ---
>>> omx/gstomx_base_filter.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> It's usually a good practice to copy-paste the actual error being fixed.
>>
>>> diff --git a/omx/gstomx_base_filter.c b/omx/gstomx_base_filter.c
>>> index 2d93f5c..7ebc52d 100644
>>> --- a/omx/gstomx_base_filter.c
>>> +++ b/omx/gstomx_base_filter.c
>>> @@ -716,7 +716,7 @@ out_flushing:
>>>
>>> if (error_msg)
>>> {
>>> - GST_ELEMENT_ERROR (self, STREAM, FAILED, (NULL), (error_msg));
>>> + GST_ELEMENT_ERROR (self, STREAM, FAILED, (NULL), ("%s", error_msg));
>>> ret = GST_FLOW_ERROR;
>>> }
>>>
>>> --
>>
>> I don't see how that could fail =/
>>
>
> sorry, could you paste below in the commit msg.. or would you rather I re-send the patch?
I'll append it. Thanks.
> -----------
> omx/gstomx_base_filter.c:763: warning: format not a string literal and no format arguments
> -----------
>
> the compiler expects first arg used in printf-like functions to be a string literal.
Ah, so it's a warning. I don't remember seeing it before, but ok. ACK.
--
Felipe Contreras
|
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:15:00
|
On Thu, Mar 18, 2010 at 12:50 AM, Rob Clark <ro...@ti...> wrote: > This improves readability by keeping knowledge of numeric port-index value > in only one place (in the element constructor), and makes it easier to > later add derived classes with different port indexes (for example > GstOmxCamera subclassing GstOmxBaseSrc, but having GstOmxBaseSrc handling a > port whose index != 0 while derived class handles port index 0 and others). This patch doesn't have much merit alone, but I guess it will be useful later on. ACK. -- Felipe Contreras |
|
From: Clark, R. <ro...@ti...> - 2010-03-23 22:14:50
|
On Mar 23, 2010, at 5:04 PM, Felipe Contreras wrote:
> On Tue, Mar 16, 2010 at 12:29 AM, Rob Clark <ro...@ti...> wrote:
>> ---
>> omx/gstomx_base_filter.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> It's usually a good practice to copy-paste the actual error being fixed.
>
>> diff --git a/omx/gstomx_base_filter.c b/omx/gstomx_base_filter.c
>> index 2d93f5c..7ebc52d 100644
>> --- a/omx/gstomx_base_filter.c
>> +++ b/omx/gstomx_base_filter.c
>> @@ -716,7 +716,7 @@ out_flushing:
>>
>> if (error_msg)
>> {
>> - GST_ELEMENT_ERROR (self, STREAM, FAILED, (NULL), (error_msg));
>> + GST_ELEMENT_ERROR (self, STREAM, FAILED, (NULL), ("%s", error_msg));
>> ret = GST_FLOW_ERROR;
>> }
>>
>> --
>
> I don't see how that could fail =/
>
sorry, could you paste below in the commit msg.. or would you rather I re-send the patch?
-----------
omx/gstomx_base_filter.c:763: warning: format not a string literal and no format arguments
-----------
the compiler expects first arg used in printf-like functions to be a string literal.
BR,
-R
> --
> Felipe Contreras
|
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:13:04
|
On Thu, Mar 18, 2010 at 12:50 AM, Rob Clark <ro...@ti...> wrote: > Do OMX_GetParameter(PortDefinition) inside g_omx_port_setup(), rather than > duplicating same code in each caller. Nice! ACK. -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:11:15
|
On Thu, Mar 18, 2010 at 12:50 AM, Rob Clark <ro...@ti...> wrote: > The port objects are constructed up in element constructor (so they are > valid at any point in the element's lifecycle), and bound to a port_index > at construction time, rather than getting constructed in setup_ports. > > This will be useful later to enable properties that directly set port > params. Looks fine to me but I recall making some comments on previous revisions. Did you forget to mention that in the commit message? -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:06:44
|
On Tue, Mar 16, 2010 at 12:29 AM, Rob Clark <ro...@ti...> wrote: > Boilerplate macros rebased on latest gst-openmax from fd.o > > (and also a fix for small compile error that started when I pulled latest gstreamer and/or common) I still have some doubts about the compile error fix, but in general all these patches look ok. Will apply ASAP. > Rob Clark (3): > A small fix for compile error with latest gstreamer > Add GSTOMX_BOILERPLATE macros > SMP safety for _get_type() functions -- Felipe Contreras |
|
From: Felipe C. <fel...@gm...> - 2010-03-23 22:04:47
|
On Tue, Mar 16, 2010 at 12:29 AM, Rob Clark <ro...@ti...> wrote:
> ---
> omx/gstomx_base_filter.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
It's usually a good practice to copy-paste the actual error being fixed.
> diff --git a/omx/gstomx_base_filter.c b/omx/gstomx_base_filter.c
> index 2d93f5c..7ebc52d 100644
> --- a/omx/gstomx_base_filter.c
> +++ b/omx/gstomx_base_filter.c
> @@ -716,7 +716,7 @@ out_flushing:
>
> if (error_msg)
> {
> - GST_ELEMENT_ERROR (self, STREAM, FAILED, (NULL), (error_msg));
> + GST_ELEMENT_ERROR (self, STREAM, FAILED, (NULL), ("%s", error_msg));
> ret = GST_FLOW_ERROR;
> }
>
> --
I don't see how that could fail =/
--
Felipe Contreras
|
3 messages has been excluded from this view by a project administrator.
