Re: [gst-cvs] mnauw gst-plugins-base: gst-plugins-base/ gst-plugins-base/gst-libs/gst/video/

[David Schleef]

On Mon, Jul 14, 2008 at 10:06:40AM -0700, [hidden email] wrote:
> CVS Root:       /cvs/gstreamer
> Module:         gst-plugins-base
> Changes by:     mnauw
> Date:           Mon Jul 14 2008  17:06:40 UTC
>
> Log message:
> * gst-libs/gst/video/video.c: (gst_video_format_parse_caps):
> Video format can also be conveniently determined from (many)
> non-fixed caps.

This change is wrong.  Please revert it.  The GstVideoFormat code
is meant to deal with fully described video formats only.

Also, it changes ABI.



dave...


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Mark Nauwelaerts-2]

David Schleef wrote:

> On Mon, Jul 14, 2008 at 10:06:40AM -0700, [hidden email] wrote:
>> CVS Root:       /cvs/gstreamer
>> Module:         gst-plugins-base
>> Changes by:     mnauw
>> Date:           Mon Jul 14 2008  17:06:40 UTC
>>
>> Log message:
>> * gst-libs/gst/video/video.c: (gst_video_format_parse_caps):
>> Video format can also be conveniently determined from (many)
>> non-fixed caps.
>
> This change is wrong.  Please revert it.  The GstVideoFormat code
> is meant to deal with fully described video formats only.

It may be meant to deal with fully described formats only,
nevertheless the same code can perfectly determine a GstVideoFormat based on
caps' fourcc code, rgb masks etc without having a fixed width and height
(and the fact that one could pass NULL width and height output parameters if one
is not interested in it was already the case; I just happened to document that
it was so).

>
> Also, it changes ABI.

It might change API in that it would now return TRUE for a non-fixed caps.
However, typical use now calls it with non-NULL width and height anyway, so it
might be changed to still return FALSE in that case while allowing non-fixed
caps if passed with NULL width and height output parameters.

If not finding that a solution, what would then be an acceptable way to re-use
exactly the same code to determine format for a non-fixed width, height etc
(which is irrelevant in the format determination) ?  Add some other API function
(_full/_non_fixed ?) that internally calls upon the same (slightly re-factored)
code.  After all, it's nice code, so let's use it as much as possible :)

Mark.


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[David Schleef]

On Tue, Jul 15, 2008 at 11:12:13AM +0200, Mark Nauwelaerts wrote:
> > Also, it changes ABI.
>
> It might change API in that it would now return TRUE for a non-fixed caps.

Bingo.  This behavior is relied upon in other areas of code.

> If not finding that a solution, what would then be an acceptable way to re-use
> exactly the same code to determine format for a non-fixed width, height etc
> (which is irrelevant in the format determination) ?  Add some other API function
> (_full/_non_fixed ?) that internally calls upon the same (slightly re-factored)
> code.  After all, it's nice code, so let's use it as much as possible :)

You'd need to write a lot of code to do this.  Basically, for each
parameter, write a function that walks the caps structures and checks
that each structure has the same fixed parameter.  It's not as simple
as not checking for fixed caps.

I don't see much usefulness in this.  In GStreamer elements, you're
either extracting information from fixed caps, as in a setcaps()
function, or you're dealing with a potentially complex caps description
that should be handled fully instead of using shortcuts.



dave...


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Mark Nauwelaerts-2]

David Schleef wrote:
> On Tue, Jul 15, 2008 at 11:12:13AM +0200, Mark Nauwelaerts wrote:
>>> Also, it changes ABI.
>> It might change API in that it would now return TRUE for a non-fixed caps.
>
> Bingo.  This behavior is relied upon in other areas of code.

I would not have expected that with the required fixed caps input in the 3 cases
in set_caps or get_unit_size in -good ?  Though ok, maybe elsewhere.

>> If not finding that a solution, what would then be an acceptable way to re-use
>> exactly the same code to determine format for a non-fixed width, height etc
>> (which is irrelevant in the format determination) ?  Add some other API function
>> (_full/_non_fixed ?) that internally calls upon the same (slightly re-factored)
>> code.  After all, it's nice code, so let's use it as much as possible :)
>
> You'd need to write a lot of code to do this.  Basically, for each
> parameter, write a function that walks the caps structures and checks
> that each structure has the same fixed parameter.  It's not as simple
> as not checking for fixed caps.

walking caps structures ?; the plural is not possible nor needed since it is
still verified now that the caps exactly holds 1 structure.  The code as it is
will then FALSE out if any of the relevant parameters in that 1 structure turn
out to hold a list or so (e.g. fourcc).  So FALSE still indicates failure with
"ambiguous/insufficiently fixed caps", as it does now with non-fixed caps (and
again; non-NULL width and height output parameters can still be made to enforce
fixed caps requirement).
On the other hand, if it does not run into a FALSE (all _get_int etc succeeded),
it means the format is unambiguously determined.

> I don't see much usefulness in this.  In GStreamer elements, you're
> either extracting information from fixed caps, as in a setcaps()
> function, or you're dealing with a potentially complex caps description
> that should be handled fully instead of using shortcuts.

A use-case; a sink setcaps triggers some caps negotiation between current
element and its peer, a get_allowed_caps on src typically returns non-fixed caps
(at least in the width, height, framerate, pixel-aspect-ratio and
whatever-may-turn-up part).  A gst_caps_normalize then yields a caps each
structure of which can be separately (caps encapsulated) fed into _parse_caps to
reduce this to a GstVideoFormat, with which the element can decide if it can
handle this format.
The "shortcut" in this is only that a library function _parse_caps can be used
easily without first having to add more code to get rid of whatever
not-so-interesting non-fixed fields there may be (e.g. fixating, removing, e.g.
with a copy-and-paste job of gst_pad_default_fixate/fixate_value).

Mark.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[David Schleef]

On Wed, Jul 16, 2008 at 12:08:57AM +0200, Mark Nauwelaerts wrote:

> A use-case; a sink setcaps triggers some caps negotiation between current
> element and its peer, a get_allowed_caps on src typically returns non-fixed
> caps (at least in the width, height, framerate, pixel-aspect-ratio and
> whatever-may-turn-up part).  A gst_caps_normalize then yields a caps each
> structure of which can be separately (caps encapsulated) fed into
> _parse_caps to reduce this to a GstVideoFormat, with which the element can
> decide if it can handle this format.
> The "shortcut" in this is only that a library function _parse_caps can be
> used easily without first having to add more code to get rid of whatever
> not-so-interesting non-fixed fields there may be (e.g. fixating, removing,
> e.g. with a copy-and-paste job of gst_pad_default_fixate/fixate_value).

If this is what you want, then you should create a proposed patch
and put it in bugzilla, for people to discuss.  This is significantly
outside of what GstVideoFormat does, and designing sane API around
caps is a difficult task.



dave...


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Stefan Sauer]

[hidden email] schrieb:

> CVS Root:       /cvs/gstreamer
> Module:         gst-plugins-base
> Changes by:     mnauw
> Date:           Mon Jul 14 2008  17:06:40 UTC
>
> Log message:
> * gst-libs/gst/video/video.c: (gst_video_format_parse_caps):
> Video format can also be conveniently determined from (many)
> non-fixed caps.
>
> Modified files:
>     .               : ChangeLog
>     gst-libs/gst/video: video.c
>
> Links:
> http://freedesktop.org/cgi-bin/viewcvs.cgi/gstreamer/gst-plugins-base/ChangeLog.diff?r1=1.4050&r2=1.4051
> http://freedesktop.org/cgi-bin/viewcvs.cgi/gstreamer/gst-plugins-base/gst-libs/gst/video/video.c.diff?r1=1.24&r2=1.25

<nitpick>
GST_CAPS_IS_SIMPLE(caps);

does that too.
</nitpick>

Stefan

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel