Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_infoframe: add support for AVI InfoFrames

["Ser, Simon" <simon.ser@intel.com>]

On Thu, 2019-08-15 at 14:30 +0300, Arkadiusz Hiler wrote:
> On Mon, Jul 22, 2019 at 06:01:26PM +0300, Simon Ser wrote:
> > This commit adds partial support for AVI InfoFrames. Only bytes 1 and 2 of the
> > InfoFrame payload are parsed.
> > 
> > These bytes contain (among other things) information about the aspect ratio of
> > the frames.
> > 
> > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > ---
> >  lib/igt_infoframe.c | 30 ++++++++++++++++++++++++++++
> >  lib/igt_infoframe.h | 48 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 78 insertions(+)
> > 
> > diff --git a/lib/igt_infoframe.c b/lib/igt_infoframe.c
> > index 7e4fb45881a6..d8a2e609573c 100644
> > --- a/lib/igt_infoframe.c
> > +++ b/lib/igt_infoframe.c
> > @@ -27,6 +27,7 @@
> >  
> >  #include <string.h>
> >  
> > +#include "igt_core.h"
> >  #include "igt_infoframe.h"
> >  
> >  /**
> > @@ -61,6 +62,35 @@ static const int sample_sizes[] = {
> >  
> >  static const size_t sample_sizes_len = sizeof(sample_sizes) / sizeof(sample_sizes[0]);
> >  
> > +bool infoframe_avi_parse(struct infoframe_avi *infoframe, int version,
> > +			 const uint8_t *buf, size_t buf_size)
> > +{
> > +	memset(infoframe, 0, sizeof(*infoframe));
> > +
> > +	switch (version) {
> > +	case 2:
> > +	case 3:
> > +	case 4:
> > +		break; /* supported */
> > +	default:
> > +		igt_debug("Unsuppported AVI InfoFrame version: %d\n", version);
> > +		return false;
> > +	}
> > +
> > +	if (buf_size < 13)
> > +		return false;
> > +
> > +	infoframe->rgb_ycbcr = buf[0] >> 5;
> > +	infoframe->scan = buf[0] & 0x3;
> > +
> > +	infoframe->colorimetry = buf[1] >> 7;
> 
> Shouldn't it be >> 6? It's 2 last bits.

Indeed, good catch!

> > +	infoframe->picture_aspect_ratio = (buf[1] >> 4) & 0x3;
> > +	infoframe->active_aspect_ratio = buf[1] & 0xF;
> > +	infoframe->vic = buf[3];
> > +
> > +	return true;
> > +}
> > +
> >  bool infoframe_audio_parse(struct infoframe_audio *infoframe, int version,
> >  			   const uint8_t *buf, size_t buf_size)
> >  {
> > diff --git a/lib/igt_infoframe.h b/lib/igt_infoframe.h
> > index 35daa3ea169d..056cdad6072a 100644
> > --- a/lib/igt_infoframe.h
> > +++ b/lib/igt_infoframe.h
> > @@ -32,6 +32,52 @@
> >  #include <stddef.h>
> >  #include <stdint.h>
> >  
> > +enum infoframe_avi_rgb_ycbcr {
> > +	INFOFRAME_AVI_RGB = 0,
> > +	INFOFRAME_AVI_YCBCR422 = 1,
> > +	INFOFRAME_AVI_YCBCR444 = 2,
> > +	INFOFRAME_AVI_YCBCR420 = 3,
> > +	INFOFRAME_AVI_IDO_DEFINED = 7,
> > +};
> > +
> > +enum infoframe_avi_scan {
> > +	INFOFRAME_AVI_SCAN_UNSPECIFIED = 0,
> > +	INFOFRAME_AVI_OVERSCAN = 1,
> > +	INFOFRAME_AVI_UNDERSCAN = 2,
> > +};
> > +
> > +enum infoframe_avi_colorimetry {
> > +	INFOFRAME_AVI_COLORIMETRY_UNSPECIFIED = 0,
> > +	INFOFRAME_AVI_SMPTE_170M = 1,
> > +	INFOFRAME_AVI_ITUR_BT709 = 2,
> > +	INFOFRAME_AVI_COLORIMETRY_EXTENDED = 3,
> > +};
> > +
> > +enum infoframe_avi_picture_aspect_ratio {
> > +	INFOFRAME_AVI_PIC_AR_UNSPECIFIED = 0,
> > +	INFOFRAME_AVI_PIC_AR_4_3 = 1,
> > +	INFOFRAME_AVI_PIC_AR_16_9 = 2,
> > +};
> > +
> > +enum infoframe_avi_active_aspect_ratio {
> > +	INFOFRAME_AVI_ACT_AR_PIC = 0, /* same as picture aspect ratio */
> 
> Shouldn't this be 8? I believe it is 0b1000.

Eh, you're right!

> With the above addressed
> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Thanks for the review
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev