[gstreamer-bugs] [Bug 622276] Add an h263parse element

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #2 from Arun Raghavan <arun at accosted.net> 2010-06-28 09:15:33 UTC ---
(In reply to comment #1)
> Review of attachment 164206 [details]:
> 
> ::: gst/h263parse/h263parse.c
> @@ +376,3 @@
> +gst_h263_parse_get_params (GstH263Parse * h263parse, H263Params * params,
> +    gboolean fast)
> +{
> 
> it would be a bit cleaner if this was in a separate file ? Along with all the
> parsing-specific structures. Then it would leave only the gst-specific parts in
> gsth263parse.[ch]

Can do.


> @@ +808,3 @@
> +
> +static gint
> +gst_h263_parse_get_profile (GstH263Parse * h263parse)
> 
> Same thing, this could be moved to a separate file and just take a H263Params
> structure as input.
> 
> @@ +930,3 @@
> +
> +static gint
> +gst_h263_parse_get_level (GstH263Parse * h263parse)
> 
> Same comment as for get_profile

Can move to a separate file. get_level needs to take a GstH263Parse* since the
bitrate isn't part of params (and IMHO should not be, since it basically
represents stream params parsed out of the header). I let get_profile also take
the same parameter for consistency, but I'm open to changing this.


> @@ +1081,3 @@
> +    if (h263parse->profile != -1)
> +      gst_caps_set_simple (caps, "profile", G_TYPE_STRING,
> +          profiles[h263parse->profile], NULL);
> 
> If profile is only going to be a numerical value, let's keep it as an integer.
> 
> @@ +1086,3 @@
> +    if (h263parse->level != -1) {
> +      level_idx = (h263parse->level / 10) + (h263parse->level % 10) - 1;
> +      gst_caps_set_simple (caps, "level", G_TYPE_STRING, levels[level_idx],
> 
> Same comment as for profile.

Since some codecs have non-numeric profiles/levels, I figured we should just
keep them all as strings instead of having to worry about which codec has a
string profile vs. which has an integer profile. Again, I'm not too religious
about this, so if the common feeling is to go with integers here, I'll change
it.


> ::: gst/h263parse/h263parse.h
> @@ +67,3 @@
> +  H263_OPTION_DPS_MODE  = 1 << 14
> +} H263OptionalFeatures;
> +
> 
> Since this is a plugin, there's maybe no need to have this flag definition here
> 
> @@ +104,3 @@
> +  UUI_IS_01,
> +} H263UUI;
> +
> 
> Same comment for all structures above.
> 
> @@ +128,3 @@
> +  H263ParseState        state;
> +} H263Params;
> +
> 
> These too

Since the header is never installed, will this make a difference?

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