Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Sakari,
Thanks for the patch.
On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> More flexible and extensible syntax for media-ctl which allows better usage
> of the selection API.
[snip]
> diff --git a/src/options.c b/src/options.c
> index 60cf6d5..4d9d48f 100644
> --- a/src/options.c
> +++ b/src/options.c
> @@ -53,12 +53,15 @@ static void usage(const char *argv0, int verbose)
> printf("\n");
> printf("Links and formats are defined as\n");
> printf("\tlink= pad, '->', pad, '[', flags, ']' ;\n");
> - printf("\tformat = pad, '[', fcc, ' ', size, [ ' ', crop ], [ '
> ', '@', frame interval ], ']' ;\n");
> + printf("\tformat = pad, '[', formats ']' ;\n");
> + printf("\tformats = formats ',' formats ;\n");
> + printf("\tformats = fmt | crop | frame interval ;\n");
That's not a valid EBNF. I'm not an expert on the subject, but I think the
following is better.
formats = format { ' ', formats }
format = fmt | crop | frame interval
> + printf("\fmt = 'fmt:', fcc, '/', size ;\n");
format, formats and fmt are becoming confusing. A different name for 'formats'
would be good.
I find the '/' a bit confusing compared to the ' ' (but I think you find the
space confusing compared to '/' :-)). I also wonder whether we shouldn't just
drop 'fmt:', as there can be a single format only.
> printf("\tpad = entity, ':', pad number ;\n");
> printf("\tentity = entity number | ( '\"', entity name, '\"' )
> ;\n");
> printf("\tsize= width, 'x', height ;\n");
> - printf("\tcrop= '(', left, ',', top, ')', '/', size ;\n");
> - printf("\tframe interval = numerator, '/', denominator ;\n");
> + printf("\tcrop= 'crop.actual:', left, ',', top, '/', size
> ;\n");
Would it make sense to make .actual implicit ? Both 'crop' and 'crop.actual'
would be supported by the parser. The code would be more generic if the target
was parsed in a generic way, not associated with the rectangle name.
I would keep the parenthesis around the (top,left) coordinates. You could then
define
rectangle = '(', left, ',', top, ')', '/', size
crop = 'crop' [ '.', target ] ':', rectangle
target = 'actual'
or something similar.
What about also keeping compatibility with the existing syntax (both for
formats and crop rectangles) ? That shouldn't be too difficult in the parser,
crop rectangles start with a '(', and formats come first. We would then have
rectangle = '(', left, ',', top, ')', '/', size
crop = [ 'crop' [ '.', target ] ':' ], rectangle
target = 'actual'
> + printf("\tframe interval = '@', numerator, '/', denominator ;\n");
> printf("where the fields are\n");
> printf("\tentity number Entity numeric identifier\n");
> printf("\tentity name Entity name (string) \n");
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Laurent,
On Sat, May 05, 2012 at 02:22:26PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thanks for the patch.
Thanks for the review!
> On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> > More flexible and extensible syntax for media-ctl which allows better usage
> > of the selection API.
>
> [snip]
>
> > diff --git a/src/options.c b/src/options.c
> > index 60cf6d5..4d9d48f 100644
> > --- a/src/options.c
> > +++ b/src/options.c
> > @@ -53,12 +53,15 @@ static void usage(const char *argv0, int verbose)
> > printf("\n");
> > printf("Links and formats are defined as\n");
> > printf("\tlink= pad, '->', pad, '[', flags, ']' ;\n");
> > - printf("\tformat = pad, '[', fcc, ' ', size, [ ' ', crop ], [ '
> > ', '@', frame interval ], ']' ;\n");
> > + printf("\tformat = pad, '[', formats ']' ;\n");
> > + printf("\tformats = formats ',' formats ;\n");
> > + printf("\tformats = fmt | crop | frame interval ;\n");
>
> That's not a valid EBNF. I'm not an expert on the subject, but I think the
> following is better.
>
> formats = format { ' ', formats }
> format = fmt | crop | frame interval
I'm fine with that change.
> > + printf("\fmt = 'fmt:', fcc, '/', size ;\n");
>
> format, formats and fmt are becoming confusing. A different name for
> 'formats'
> would be good.
I agree but I didn't immediately come up with something better.
The pixel format and the image size at the pad are clearly format
(VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
format.
I see them different kinds of properties of pads. That suggests we might be
better renaming the option (-f) to something else as well.
> I find the '/' a bit confusing compared to the ' ' (but I think you find the
> space confusing compared to '/' :-)). I also wonder whether we shouldn't just
> drop 'fmt:', as there can be a single format only.
You can set it multiple times, or you may not set it at all. That's why I
think we should explicitly say it's the format.
> > printf("\tpad = entity, ':', pad number ;\n");
> > printf("\tentity = entity number | ( '\"', entity name, '\"' )
> > ;\n");
> > printf("\tsize= width, 'x', height ;\n");
> > - printf("\tcrop= '(', left, ',', top, ')', '/', size ;\n");
> > - printf("\tframe interval = numerator, '/', denominator ;\n");
> > + printf("\tcrop= 'crop.actual:', left, ',', top, '/', size
> > ;\n");
>
> Would it make sense to make .actual implicit ? Both 'crop' and 'crop.actual'
> would be supported by the parser. The code would be more generic if the
> target
> was parsed in a generic way, not associated with the rectangle name.
I've been also thinking does the actual / active really signify something,
or should that be even dropped form the V4L2 / V4L2 subdev interface
definitions.
> I would keep the parenthesis around the (top,left) coordinates. You could
> then
> define
>
> rectangle = '(', left, ',', top, ')', '/', size
> crop = 'crop' [ '.', target ] ':', rectangle
> target = 'actual'
>
> or something similar.
Sounds good to me.
> What about also keeping compatibility with the existing syntax (both for
> formats and crop rectangles) ? That shouldn't be too difficult in the parser,
> crop rectangles start with a '(', and formats come first. We would then have
>
> rectangle = '(', left, ',', top, ')', '/', size
> crop = [ 'crop' [ '.', target ] ':' ], rectangle
> target = 'actual'
I'll see what I can do. We still should drop the documentation for this so
that people will stop writing rules that look like that.
Cheers,
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Sakari,
On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
> On Sat, May 05, 2012 at 02:22:26PM +0200, Laurent Pinchart wrote:
> > On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> > > More flexible and extensible syntax for media-ctl which allows better
> > > usage
> > > of the selection API.
> >
> > [snip]
> >
> > > diff --git a/src/options.c b/src/options.c
> > > index 60cf6d5..4d9d48f 100644
> > > --- a/src/options.c
> > > +++ b/src/options.c
> > > @@ -53,12 +53,15 @@ static void usage(const char *argv0, int verbose)
> > >
> > > printf("\n");
> > > printf("Links and formats are defined as\n");
> > > printf("\tlink= pad, '->', pad, '[', flags, ']' ;\n");
> > >
> > > - printf("\tformat = pad, '[', fcc, ' ', size, [ ' ', crop ],
[
> > > '
> > > ', '@', frame interval ], ']' ;\n");
> > > + printf("\tformat = pad, '[', formats ']' ;\n");
> > > + printf("\tformats = formats ',' formats ;\n");
> > > + printf("\tformats = fmt | crop | frame interval ;\n");
> >
> > That's not a valid EBNF. I'm not an expert on the subject, but I think the
> > following is better.
> >
> > formats = format { ' ', formats }
> > format = fmt | crop | frame interval
>
> I'm fine with that change.
>
> > > + printf("\fmt = 'fmt:', fcc, '/', size ;\n");
> >
> > format, formats and fmt are becoming confusing. A different name for
> > 'formats' would be good.
>
> I agree but I didn't immediately come up with something better.
I haven't found a better option at first sight either, let's try to sleep on
it.
> The pixel format and the image size at the pad are clearly format
> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
> format.
>
> I see them different kinds of properties of pads. That suggests we might be
> better renaming the option (-f) to something else as well.
You like breaking interfaces, don't you ? :-D
> > I find the '/' a bit confusing compared to the ' ' (but I think you find
> > the space confusing compared to '/' :-)). I also wonder whether we
> > shouldn't just drop 'fmt:', as there can be a single format only.
>
> You can set it multiple times, or you may not set it at all. That's why I
> think we should explicitly say it's the format.
Not at all makes sense, but why would you set it multiple times ?
> > > printf("\tpad = entity, ':', pad number ;\n");
> > > printf("\tentity = entity number | ( '\"', entity name, '\"'
> > > )
> > >
> > > ;\n");
> > >
> > > printf("\tsize= width, 'x', height ;\n");
> > >
> > > - printf("\tcrop= '(', left, ',', top, ')', '/', size ;
\n");
> > > - printf("\tframe interval = numerator, '/', denominator ;\n");
> > > + printf("\tcrop= 'crop.actual:', left, ',', top, '/', size
> > > ;\n");
> >
> > Would it make sense to make .actual implicit ? Both 'crop' and
> > 'crop.actual' would be supported by the parser. The code would be more
> > generic if the target was parsed in a generic way, not associated with
> > the rectangle name.
> I've been also thinking does the actual / active really signify something,
> or should that be even dropped form the V4L2 / V4L2 subdev interface
> definitions.
>
> > I would keep the parenthesis around the (top,left) coordinates. You could
> > then define
> >
> > rectangle = '(', left, ',', top, ')', '/', size
> > crop = 'crop' [ '.', target ] ':', rectangle
> > target = 'actual'
> >
> > or something similar.
>
> Sounds good to me.
>
> > What about also keeping compatibility with the existing syntax (both for
> > formats and crop rectangles) ? That shouldn't be too difficult in the
> > parser, crop rectangles start with a '(', and formats come first. We
> > would then have
> >
> > rectangle = '(', left, ',', top, ')', '/', size
> > crop = [ 'crop' [ '.', target ] ':' ], rectangle
> > target = 'actual'
>
> I'll see what I can do. We still should drop the documentation for this so
> that people will stop writing rules that look like that.
Good point, I agree.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Laurent,
Laurent Pinchart wrote:
> On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
...
>> The pixel format and the image size at the pad are clearly format
>> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
>> format.
>>
>> I see them different kinds of properties of pads. That suggests we might be
>> better renaming the option (-f) to something else as well.
>
> You like breaking interfaces, don't you ? :-D
I thought you said we have no stable release yet. :-D
The selection interface on subdevs is currently used to change format
related things (cropping and scaling, for example) but it was one of
Sylwester's patches ("V4L: Add auto focus targets to the selections
API") that adds a focus window target to the V4L2 selection interface. I
don't see why it couldn't be present on subdevs, too. That's got nothing
to do with the image format.
I've been pondering a bit using another option to configure things
related to selections. Conveniently "-s" is free. We could leave the
crop things to -f but remove the documentation related to them.
I'm fine with keeping the things as they are for now, too, but in that
case we should recognise that -f will not be for formats only. Or we
split handling selections into separate options, but I don't like that
idea either.
>>> I find the '/' a bit confusing compared to the ' ' (but I think you find
>>> the space confusing compared to '/' :-)). I also wonder whether we
>>> shouldn't just drop 'fmt:', as there can be a single format only.
>>
>> You can set it multiple times, or you may not set it at all. That's why I
>> think we should explicitly say it's the format.
>
> Not at all makes sense, but why would you set it multiple times ?
I guess that's not a very practical use case, albeit there may be
dependencies between the two: Guennadi had a piece of hardware where the
hardware cropping or scaling capabilities depended on the format. But
not setting it at all definitely is a valid use case.
Kind regards,
--
Sakari Ailus
sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Sakari,
On Sunday 06 May 2012 21:14:02 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
> ...
>
> >> The pixel format and the image size at the pad are clearly format
> >> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
> >> format.
> >>
> >> I see them different kinds of properties of pads. That suggests we might
> >> be better renaming the option (-f) to something else as well.
> >
> > You like breaking interfaces, don't you ? :-D
>
> I thought you said we have no stable release yet. :-D
>
> The selection interface on subdevs is currently used to change format
> related things (cropping and scaling, for example) but it was one of
> Sylwester's patches ("V4L: Add auto focus targets to the selections
> API") that adds a focus window target to the V4L2 selection interface. I
> don't see why it couldn't be present on subdevs, too. That's got nothing
> to do with the image format.
>
> I've been pondering a bit using another option to configure things
> related to selections. Conveniently "-s" is free. We could leave the
> crop things to -f but remove the documentation related to them.
It would then become much more complex to setup a complete pipeline in a
single command line, unless we completely modify the way the command line is
parsed.
What would you think about renaming -f to -V (long option --video or --v4l2) ?
media-ctl will hopefully be used for non-V4L2 devices in the future, so
subsystem-specific options will likely be needed.
> I'm fine with keeping the things as they are for now, too, but in that
> case we should recognise that -f will not be for formats only. Or we
> split handling selections into separate options, but I don't like that
> idea either.
>
> >>> I find the '/' a bit confusing compared to the ' ' (but I think you find
> >>> the space confusing compared to '/' :-)). I also wonder whether we
> >>> shouldn't just drop 'fmt:', as there can be a single format only.
> >>
> >> You can set it multiple times, or you may not set it at all. That's why I
> >> think we should explicitly say it's the format.
> >
> > Not at all makes sense, but why would you set it multiple times ?
>
> I guess that's not a very practical use case, albeit there may be
> dependencies between the two: Guennadi had a piece of hardware where the
> hardware cropping or scaling capabilities depended on the format.
We don't have a way to handle that cleanly in the V4L2 API, do we ?
> But not setting it at all definitely is a valid use case.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Laurent,
On Mon, May 07, 2012 at 12:44:45PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Sunday 06 May 2012 21:14:02 Sakari Ailus wrote:
> > Laurent Pinchart wrote:
> > > On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
> > ...
> >
> > >> The pixel format and the image size at the pad are clearly format
> > >> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
> > >> format.
> > >>
> > >> I see them different kinds of properties of pads. That suggests we might
> > >> be better renaming the option (-f) to something else as well.
> > >
> > > You like breaking interfaces, don't you ? :-D
> >
> > I thought you said we have no stable release yet. :-D
> >
> > The selection interface on subdevs is currently used to change format
> > related things (cropping and scaling, for example) but it was one of
> > Sylwester's patches ("V4L: Add auto focus targets to the selections
> > API") that adds a focus window target to the V4L2 selection interface. I
> > don't see why it couldn't be present on subdevs, too. That's got nothing
> > to do with the image format.
> >
> > I've been pondering a bit using another option to configure things
> > related to selections. Conveniently "-s" is free. We could leave the
> > crop things to -f but remove the documentation related to them.
>
> It would then become much more complex to setup a complete pipeline in a
> single command line, unless we completely modify the way the command line is
> parsed.
>
> What would you think about renaming -f to -V (long option --video or --v4l2)
> ?
> media-ctl will hopefully be used for non-V4L2 devices in the future, so
> subsystem-specific options will likely be needed.
Sounds like a very good idea to me; it solves the issue properly and makes
the V4L2 functionality better separated in media-ctl. I'll put that on top
of the existing stack.
> > I'm fine with keeping the things as they are for now, too, but in that
> > case we should recognise that -f will not be for formats only. Or we
> > split handling selections into separate options, but I don't like that
> > idea either.
> >
> > >>> I find the '/' a bit confusing compared to the ' ' (but I think you find
> > >>> the space confusing compared to '/' :-)). I also wonder whether we
> > >>> shouldn't just drop 'fmt:', as there can be a single format only.
> > >>
> > >> You can set it multiple times, or you may not set it at all. That's why I
> > >> think we should explicitly say it's the format.
> > >
> > > Not at all makes sense, but why would you set it multiple times ?
> >
> > I guess that's not a very practical use case, albeit there may be
> > dependencies between the two: Guennadi had a piece of hardware where the
> > hardware cropping or scaling capabilities depended on the format.
>
> We don't have a way to handle that cleanly in the V4L2 API, do we ?
No, not really. The user has to set the format before any selection targets
on either of the pads, wchich is a little bit against the documentation. But
this is a very special case.
Regards,
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
