Hello Hans, no need to thank me for working on it. First of all I do it as a part of a summerjob, and more importantly I quite enjoy it and intend to stay a active member of the development process after my time at Cisco is over.
Thank you for your comments. > Most fields in this struct (and in the other structs for that matter) could > do with some more documentation. I added a lot of short comments explaining the meaning of the fields, and extended the explanation part that comes before each struct definition. > > + /** RDS info fields **/ > > + bool is_rbds; /* use RBDS standard version of LUTs */ > > + uint16_t pi; > > + uint8_t ps[8]; > > Looking at rds-ctl, this contains a string, please make it 9 bytes and > always 0 terminate it! I also notice in rds-ctl that you filter the chars > for being valid ascii and if not replace them with a space. Does the spec > say anything about the encoding used for this string? Could we maybe > convert it to UTF-8 inside the library so that apps can just consume the > string? The character encoding complies with ISO/IEC 10646, so it basically already is UTF-8, and the data could be stored in a wchar_t array without conversion. Is that preferred over uint8_t? > > +/* adds a raw RDS block to decode it into RDS groups > > + * @return: bitmask with with updated fields set to 1 > > + * @rds_data: 3 bytes of raw RDS data, obtained by calling read() > > + * on RDS capable V4L2 devices */ > > +LIBV4L_PUBLIC uint32_t v4l2_rds_add(struct v4l2_rds *handle, struct > > v4l2_rds_data *rds_data); > > Unless I'm missing something, you are no defining struct v4l2_rds_data > anywhere, why not just make this a uint8_t ? The v4l2_rds_data structure is defined by v4l in the videodev2.h header, and is returned when calling the read function on a rds capable device source: http://hverkuil.home.xs4all.nl/spec/media.html#v4l2-rds-data Maybe I didn't get you point though? greetings, Konke -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
