Re: [PATCH v1 15/19] uvcvideo: Add support for VP8 special frame flags.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Pawel,

Thank you for the patch.

On Friday 30 August 2013 11:17:14 Pawel Osciak wrote:

Commit message missing :-)

> Signed-off-by: Pawel Osciak <posciak@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++++++++++-
>  drivers/media/usb/uvc/uvcvideo.h  | 10 ++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 59f57a2..0291817 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1136,6 +1136,8 @@ static int uvc_video_parse_header(struct uvc_streaming
> *stream, if (header->has_scr)
>  		header->length += 6;
> 
> +	header->buf_flags = 0;
> +

I would have moved this initialization line above right after header->length, 
to keep it before the conditional checks, but that's up to you.

>  	if (stream->cur_format->fcc == V4L2_PIX_FMT_VP8) {
>  		/* VP8 payload has 2 additional bytes of BFH. */
>  		header->length += 2;
> @@ -1147,6 +1149,16 @@ static int uvc_video_parse_header(struct
> uvc_streaming *stream, header->has_sli = data[1] & UVC_STREAM_SLI;
>  		if (header->has_sli)
>  			header->length += 2;
> +
> +		/* Codec-specific flags for v4l2_buffer. */
> +		header->buf_flags |= (data[1] & UVC_STREAM_STI) ?
> +					V4L2_BUF_FLAG_KEYFRAME : 0;

Is a keyframe the same thing as an intraframe ?

> +		header->buf_flags |= (data[2] & UVC_STREAM_VP8_PRF) ?
> +					V4L2_BUF_FLAG_PREV_FRAME : 0;
> +		header->buf_flags |= (data[2] & UVC_STREAM_VP8_ARF) ?
> +					V4L2_BUF_FLAG_ALTREF_FRAME : 0;
> +		header->buf_flags |= (data[2] & UVC_STREAM_VP8_GRF) ?
> +					V4L2_BUF_FLAG_GOLDEN_FRAME : 0;
>  	}
> 
>  	/* - bHeaderLength value can't be larger than the packet size. */
> @@ -1222,6 +1234,8 @@ static void uvc_video_decode_isoc(struct urb *urb,
> struct uvc_streaming *stream) if (ret < 0)
>  			continue;
> 
> +		buf->buf.v4l2_buf.flags |= header.buf_flags;

What about moving this line to the end of uvc_video_decode_data(), right 
before it returns successfully ?

> +
>  		/* Decode the payload data. */
>  		uvc_video_decode_data(stream, buf, mem + header.length,
>  			urb->iso_frame_desc[i].actual_length - header.length);
> @@ -1293,8 +1307,10 @@ static void uvc_video_decode_bulk(struct urb *urb,
> struct uvc_streaming *stream) */
> 
>  	/* Process video data. */
> -	if (!stream->bulk.skip_payload && buf != NULL)
> +	if (!stream->bulk.skip_payload && buf != NULL) {
>  		uvc_video_decode_data(stream, buf, mem, len);
> +		buf->buf.v4l2_buf.flags |= header.buf_flags;
> +	}
> 
>  	/* Detect the payload end by a URB smaller than the maximum size (or
>  	 * a payload size equal to the maximum) and process the header again.
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index b355b2c..fb21459 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -145,6 +145,14 @@
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
>  #define UVC_FMT_FLAG_STREAM		0x00000002
> 
> +/* v4l2_buffer codec flags */
> +#define UVC_V4L2_BUFFER_CODEC_FLAGS	(V4L2_BUF_FLAG_KEYFRAME | \
> +					 V4L2_BUF_FLAG_PFRAME | \
> +					 V4L2_BUF_FLAG_BFRAME | \
> +					 V4L2_BUF_FLAG_PREV_FRAME | \
> +					 V4L2_BUF_FLAG_GOLDEN_FRAME | \
> +					 V4L2_BUF_FLAG_ALTREF_FRAME)

This isn't used in this patch, could you move it to the patch that needs it ?

> +
>  /* ------------------------------------------------------------------------
> * Structures.
>   */
> @@ -472,6 +480,8 @@ struct uvc_payload_header {
> 
>  	int length;
>  	int payload_size;
> +
> +	__u32 buf_flags; /* v4l2_buffer flags */
>  };
> 
>  struct uvc_streaming {
-- 
Regards,

Laurent Pinchart