lkml.org 
[lkml]   [2015]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] [media] hdmi: added functions for MPEG InfoFrames
On Sat, Nov 14, 2015 at 07:38:19PM +0100, Enric Balletbo i Serra wrote:
> The MPEG Source (MS) InfoFrame is in EIA/CEA-861B. It describes aspects of
> the compressed video stream that were used to produce the uncompressed
> video.
>
> The patch adds functions to work with MPEG InfoFrames.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> drivers/video/hdmi.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/hdmi.h | 24 ++++++++
> 2 files changed, 180 insertions(+)

According to the CEA specification a source is expected to send this
type of infoframe once per video frame. I'm curious how you envision
this to be ensured. Would hardware provide a mechanism to store this
data and send the infoframe automatically? How would you ensure that
updates sent to the hardware match the upcoming frame?

> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
[...]
> @@ -435,6 +510,8 @@ hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size)
> length = hdmi_vendor_any_infoframe_pack(&frame->vendor,
> buffer, size);
> break;
> + case HDMI_INFOFRAME_TYPE_MPEG:
> + length = hdmi_mpeg_infoframe_pack(&frame->mpeg, buffer, size);

Missing "break;"?

> @@ -899,6 +978,41 @@ static void hdmi_audio_infoframe_log(const char *level,
> frame->downmix_inhibit ? "Yes" : "No");
> }
>
> +static const char *hdmi_mpeg_picture_get_name(enum hdmi_mpeg_picture_type type)
> +{
> + switch (type) {
> + case HDMI_MPEG_PICTURE_TYPE_UNKNOWN:
> + return "Unknown";
> + case HDMI_MPEG_PICTURE_TYPE_I:
> + return "Intra-coded picture";
> + case HDMI_MPEG_PICTURE_TYPE_B:
> + return "Bi-predictive picture";
> + case HDMI_MPEG_PICTURE_TYPE_P:
> + return "Predicted picture";
> + }

I'd have chosen the slightly more canonical "I-Frame", "P-Frame",
"B-Frame" here, but that's not a strong objection.

> + return "Reserved";

There really can't be another value here, so I think this should either
return NULL or even go as far as let it crash. There should be no way
for the invalid value to make it this far.

> +static int hdmi_mpeg_infoframe_unpack(struct hdmi_mpeg_infoframe *frame,
> + void *buffer)
> +{
> + u8 *ptr = buffer;
> +
> + if (ptr[0] != HDMI_INFOFRAME_TYPE_MPEG ||
> + ptr[1] != 1 ||
> + ptr[2] != HDMI_MPEG_INFOFRAME_SIZE) {
> + return -EINVAL;
> + }

There's no need for the braces here.

> @@ -314,6 +336,7 @@ union hdmi_vendor_any_infoframe {
> * @spd: spd infoframe
> * @vendor: union of all vendor infoframes
> * @audio: audio infoframe
> + * @mpeg: mpeg infoframe

s/mpeg/MPEG/

Thierry
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2015-11-16 13:21    [W:0.036 / U:13.192 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site