Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.ailus@xxxxxx	jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux