* [PATCH] media: v4l2: Initialize mpeg slice controls
@ 2019-05-03 11:42 Boris Brezillon
2019-05-29 14:41 ` Hans Verkuil
0 siblings, 1 reply; 7+ messages in thread
From: Boris Brezillon @ 2019-05-03 11:42 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, linux-media
Cc: kernel, Boris Brezillon
Make sure the default value at least passes the std_validate() tests.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b1ae2e555c68..19d40cc6e565 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1461,7 +1461,14 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
union v4l2_ctrl_ptr ptr)
{
- switch (ctrl->type) {
+ struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
+
+ /*
+ * The cast is needed to get rid of a gcc warning complaining that
+ * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the
+ * v4l2_ctrl_type enum.
+ */
+ switch ((u32)ctrl->type) {
case V4L2_CTRL_TYPE_STRING:
idx *= ctrl->elem_size;
memset(ptr.p_char + idx, ' ', ctrl->minimum);
@@ -1486,6 +1493,17 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
case V4L2_CTRL_TYPE_U32:
ptr.p_u32[idx] = ctrl->default_value;
break;
+ case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
+ p_mpeg2_slice_params = ptr.p;
+ /* 4:2:0 */
+ p_mpeg2_slice_params->sequence.chroma_format = 1;
+ /* 8 bits */
+ p_mpeg2_slice_params->picture.intra_dc_precision = 0;
+ /* interlaced top field */
+ p_mpeg2_slice_params->picture.picture_structure = 1;
+ p_mpeg2_slice_params->picture.picture_coding_type =
+ V4L2_MPEG2_PICTURE_CODING_TYPE_I;
+ break;
default:
idx *= ctrl->elem_size;
memset(ptr.p + idx, 0, ctrl->elem_size);
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] media: v4l2: Initialize mpeg slice controls
2019-05-03 11:42 [PATCH] media: v4l2: Initialize mpeg slice controls Boris Brezillon
@ 2019-05-29 14:41 ` Hans Verkuil
2019-05-29 15:36 ` Ezequiel Garcia
0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2019-05-29 14:41 UTC (permalink / raw)
To: Boris Brezillon, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus, linux-media
Cc: kernel
On 5/3/19 1:42 PM, Boris Brezillon wrote:
> Make sure the default value at least passes the std_validate() tests.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index b1ae2e555c68..19d40cc6e565 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1461,7 +1461,14 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> union v4l2_ctrl_ptr ptr)
> {
> - switch (ctrl->type) {
> + struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> +
> + /*
> + * The cast is needed to get rid of a gcc warning complaining that
> + * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the
> + * v4l2_ctrl_type enum.
> + */
> + switch ((u32)ctrl->type) {
> case V4L2_CTRL_TYPE_STRING:
> idx *= ctrl->elem_size;
> memset(ptr.p_char + idx, ' ', ctrl->minimum);
> @@ -1486,6 +1493,17 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> case V4L2_CTRL_TYPE_U32:
> ptr.p_u32[idx] = ctrl->default_value;
> break;
> + case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> + p_mpeg2_slice_params = ptr.p;
> + /* 4:2:0 */
> + p_mpeg2_slice_params->sequence.chroma_format = 1;
> + /* 8 bits */
> + p_mpeg2_slice_params->picture.intra_dc_precision = 0;
> + /* interlaced top field */
> + p_mpeg2_slice_params->picture.picture_structure = 1;
> + p_mpeg2_slice_params->picture.picture_coding_type =
> + V4L2_MPEG2_PICTURE_CODING_TYPE_I;
Oops, this isn't complete. It should still zero the p_mpeg2_slice_params
struct first. Right now any fields not explicitly set just have whatever
was in memory.
Can you post a patch fixing this?
Regards,
Hans
> + break;
> default:
> idx *= ctrl->elem_size;
> memset(ptr.p + idx, 0, ctrl->elem_size);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: v4l2: Initialize mpeg slice controls
2019-05-29 14:41 ` Hans Verkuil
@ 2019-05-29 15:36 ` Ezequiel Garcia
2019-05-29 15:42 ` Hans Verkuil
0 siblings, 1 reply; 7+ messages in thread
From: Ezequiel Garcia @ 2019-05-29 15:36 UTC (permalink / raw)
To: Hans Verkuil, Boris Brezillon, Mauro Carvalho Chehab,
Hans Verkuil, Laurent Pinchart, Sakari Ailus, linux-media
Cc: kernel
On Wed, 2019-05-29 at 16:41 +0200, Hans Verkuil wrote:
> On 5/3/19 1:42 PM, Boris Brezillon wrote:
> > Make sure the default value at least passes the std_validate() tests.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > drivers/media/v4l2-core/v4l2-ctrls.c | 20 +++++++++++++++++++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index b1ae2e555c68..19d40cc6e565 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1461,7 +1461,14 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> > static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > union v4l2_ctrl_ptr ptr)
> > {
> > - switch (ctrl->type) {
> > + struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> > +
> > + /*
> > + * The cast is needed to get rid of a gcc warning complaining that
> > + * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the
> > + * v4l2_ctrl_type enum.
> > + */
> > + switch ((u32)ctrl->type) {
> > case V4L2_CTRL_TYPE_STRING:
> > idx *= ctrl->elem_size;
> > memset(ptr.p_char + idx, ' ', ctrl->minimum);
> > @@ -1486,6 +1493,17 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > case V4L2_CTRL_TYPE_U32:
> > ptr.p_u32[idx] = ctrl->default_value;
> > break;
> > + case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> > + p_mpeg2_slice_params = ptr.p;
> > + /* 4:2:0 */
> > + p_mpeg2_slice_params->sequence.chroma_format = 1;
> > + /* 8 bits */
> > + p_mpeg2_slice_params->picture.intra_dc_precision = 0;
> > + /* interlaced top field */
> > + p_mpeg2_slice_params->picture.picture_structure = 1;
> > + p_mpeg2_slice_params->picture.picture_coding_type =
> > + V4L2_MPEG2_PICTURE_CODING_TYPE_I;
>
> Oops, this isn't complete. It should still zero the p_mpeg2_slice_params
> struct first. Right now any fields not explicitly set just have whatever
> was in memory.
>
> Can you post a patch fixing this?
>
>
I was wondering if we want to zero all the cases, and not just
the struct types ones.
Or a different approach would be to define statically-allocated default
structs, and init-assign to them. Although this take more space,
and has no benefits, except maybe code clarity.
OTOH, I can push a quick fix if you want, just zeroing this struct.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: v4l2: Initialize mpeg slice controls
2019-05-29 15:36 ` Ezequiel Garcia
@ 2019-05-29 15:42 ` Hans Verkuil
2019-05-29 16:06 ` Boris Brezillon
0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2019-05-29 15:42 UTC (permalink / raw)
To: Ezequiel Garcia, Boris Brezillon, Mauro Carvalho Chehab,
Hans Verkuil, Laurent Pinchart, Sakari Ailus, linux-media
Cc: kernel
On 5/29/19 5:36 PM, Ezequiel Garcia wrote:
> On Wed, 2019-05-29 at 16:41 +0200, Hans Verkuil wrote:
>> On 5/3/19 1:42 PM, Boris Brezillon wrote:
>>> Make sure the default value at least passes the std_validate() tests.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>>> ---
>>> drivers/media/v4l2-core/v4l2-ctrls.c | 20 +++++++++++++++++++-
>>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index b1ae2e555c68..19d40cc6e565 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -1461,7 +1461,14 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
>>> static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
>>> union v4l2_ctrl_ptr ptr)
>>> {
>>> - switch (ctrl->type) {
>>> + struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
>>> +
>>> + /*
>>> + * The cast is needed to get rid of a gcc warning complaining that
>>> + * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the
>>> + * v4l2_ctrl_type enum.
>>> + */
>>> + switch ((u32)ctrl->type) {
>>> case V4L2_CTRL_TYPE_STRING:
>>> idx *= ctrl->elem_size;
>>> memset(ptr.p_char + idx, ' ', ctrl->minimum);
>>> @@ -1486,6 +1493,17 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
>>> case V4L2_CTRL_TYPE_U32:
>>> ptr.p_u32[idx] = ctrl->default_value;
>>> break;
>>> + case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
>>> + p_mpeg2_slice_params = ptr.p;
>>> + /* 4:2:0 */
>>> + p_mpeg2_slice_params->sequence.chroma_format = 1;
>>> + /* 8 bits */
>>> + p_mpeg2_slice_params->picture.intra_dc_precision = 0;
>>> + /* interlaced top field */
>>> + p_mpeg2_slice_params->picture.picture_structure = 1;
>>> + p_mpeg2_slice_params->picture.picture_coding_type =
>>> + V4L2_MPEG2_PICTURE_CODING_TYPE_I;
>>
>> Oops, this isn't complete. It should still zero the p_mpeg2_slice_params
>> struct first. Right now any fields not explicitly set just have whatever
>> was in memory.
>>
>> Can you post a patch fixing this?
>>
>>
>
>
> I was wondering if we want to zero all the cases, and not just
> the struct types ones.
The others either overwrite the data with the default_value, or memset
the whole control (default case). It's only for these compound controls
that something special needs to be done.
The code can be restructured, though: instead of break do return in all
the simple type cases.
Then call memset followed by a new switch for the compound types where you
need to set some fields to non-zero.
>
> Or a different approach would be to define statically-allocated default
> structs, and init-assign to them. Although this take more space,
> and has no benefits, except maybe code clarity.
>
> OTOH, I can push a quick fix if you want, just zeroing this struct.
>
A quick fix is fine, but as we add new compound types here we need something
along the lines of what I described above. A statically-allocated default
struct isn't a bad idea either, but a bit overkill for the simple mpeg2_slice_params.
Regards,
Hans
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: v4l2: Initialize mpeg slice controls
2019-05-29 15:42 ` Hans Verkuil
@ 2019-05-29 16:06 ` Boris Brezillon
2019-05-29 16:59 ` Ezequiel Garcia
0 siblings, 1 reply; 7+ messages in thread
From: Boris Brezillon @ 2019-05-29 16:06 UTC (permalink / raw)
To: Hans Verkuil
Cc: Ezequiel Garcia, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus, linux-media, kernel
On Wed, 29 May 2019 17:42:58 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 5/29/19 5:36 PM, Ezequiel Garcia wrote:
> > On Wed, 2019-05-29 at 16:41 +0200, Hans Verkuil wrote:
> >> On 5/3/19 1:42 PM, Boris Brezillon wrote:
> >>> Make sure the default value at least passes the std_validate() tests.
> >>>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> >>> ---
> >>> drivers/media/v4l2-core/v4l2-ctrls.c | 20 +++++++++++++++++++-
> >>> 1 file changed, 19 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> index b1ae2e555c68..19d40cc6e565 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> @@ -1461,7 +1461,14 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> >>> static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> >>> union v4l2_ctrl_ptr ptr)
> >>> {
> >>> - switch (ctrl->type) {
> >>> + struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> >>> +
> >>> + /*
> >>> + * The cast is needed to get rid of a gcc warning complaining that
> >>> + * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the
> >>> + * v4l2_ctrl_type enum.
> >>> + */
> >>> + switch ((u32)ctrl->type) {
> >>> case V4L2_CTRL_TYPE_STRING:
> >>> idx *= ctrl->elem_size;
> >>> memset(ptr.p_char + idx, ' ', ctrl->minimum);
> >>> @@ -1486,6 +1493,17 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> >>> case V4L2_CTRL_TYPE_U32:
> >>> ptr.p_u32[idx] = ctrl->default_value;
> >>> break;
> >>> + case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> >>> + p_mpeg2_slice_params = ptr.p;
> >>> + /* 4:2:0 */
> >>> + p_mpeg2_slice_params->sequence.chroma_format = 1;
> >>> + /* 8 bits */
> >>> + p_mpeg2_slice_params->picture.intra_dc_precision = 0;
> >>> + /* interlaced top field */
> >>> + p_mpeg2_slice_params->picture.picture_structure = 1;
> >>> + p_mpeg2_slice_params->picture.picture_coding_type =
> >>> + V4L2_MPEG2_PICTURE_CODING_TYPE_I;
> >>
> >> Oops, this isn't complete. It should still zero the p_mpeg2_slice_params
> >> struct first. Right now any fields not explicitly set just have whatever
> >> was in memory.
Oops.
> >>
> >> Can you post a patch fixing this?
> >>
> >>
> >
> >
> > I was wondering if we want to zero all the cases, and not just
> > the struct types ones.
>
> The others either overwrite the data with the default_value, or memset
> the whole control (default case). It's only for these compound controls
> that something special needs to be done.
>
> The code can be restructured, though: instead of break do return in all
> the simple type cases.
>
> Then call memset followed by a new switch for the compound types where you
> need to set some fields to non-zero.
memset(0) will fix the undefined val issue, the question is, is it the
default we want? I haven't worked on MPEG2 (just posted patches
Ezequiel worked on) so I can't tell.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: v4l2: Initialize mpeg slice controls
2019-05-29 16:06 ` Boris Brezillon
@ 2019-05-29 16:59 ` Ezequiel Garcia
2019-05-29 17:06 ` Boris Brezillon
0 siblings, 1 reply; 7+ messages in thread
From: Ezequiel Garcia @ 2019-05-29 16:59 UTC (permalink / raw)
To: Boris Brezillon, Hans Verkuil
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, linux-media, kernel
On Wed, 2019-05-29 at 18:06 +0200, Boris Brezillon wrote:
> On Wed, 29 May 2019 17:42:58 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> > On 5/29/19 5:36 PM, Ezequiel Garcia wrote:
> > > On Wed, 2019-05-29 at 16:41 +0200, Hans Verkuil wrote:
> > > > On 5/3/19 1:42 PM, Boris Brezillon wrote:
> > > > > Make sure the default value at least passes the std_validate() tests.
> > > > >
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > ---
> > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 20 +++++++++++++++++++-
> > > > > 1 file changed, 19 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > > index b1ae2e555c68..19d40cc6e565 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > > @@ -1461,7 +1461,14 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> > > > > static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > > > > union v4l2_ctrl_ptr ptr)
> > > > > {
> > > > > - switch (ctrl->type) {
> > > > > + struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> > > > > +
> > > > > + /*
> > > > > + * The cast is needed to get rid of a gcc warning complaining that
> > > > > + * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the
> > > > > + * v4l2_ctrl_type enum.
> > > > > + */
> > > > > + switch ((u32)ctrl->type) {
> > > > > case V4L2_CTRL_TYPE_STRING:
> > > > > idx *= ctrl->elem_size;
> > > > > memset(ptr.p_char + idx, ' ', ctrl->minimum);
> > > > > @@ -1486,6 +1493,17 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > > > > case V4L2_CTRL_TYPE_U32:
> > > > > ptr.p_u32[idx] = ctrl->default_value;
> > > > > break;
> > > > > + case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> > > > > + p_mpeg2_slice_params = ptr.p;
> > > > > + /* 4:2:0 */
> > > > > + p_mpeg2_slice_params->sequence.chroma_format = 1;
> > > > > + /* 8 bits */
> > > > > + p_mpeg2_slice_params->picture.intra_dc_precision = 0;
> > > > > + /* interlaced top field */
> > > > > + p_mpeg2_slice_params->picture.picture_structure = 1;
> > > > > + p_mpeg2_slice_params->picture.picture_coding_type =
> > > > > + V4L2_MPEG2_PICTURE_CODING_TYPE_I;
> > > >
> > > > Oops, this isn't complete. It should still zero the p_mpeg2_slice_params
> > > > struct first. Right now any fields not explicitly set just have whatever
> > > > was in memory.
>
> Oops.
>
> > > > Can you post a patch fixing this?
> > > >
> > > >
> > >
> > > I was wondering if we want to zero all the cases, and not just
> > > the struct types ones.
> >
> > The others either overwrite the data with the default_value, or memset
> > the whole control (default case). It's only for these compound controls
> > that something special needs to be done.
> >
> > The code can be restructured, though: instead of break do return in all
> > the simple type cases.
> >
> > Then call memset followed by a new switch for the compound types where you
> > need to set some fields to non-zero.
>
> memset(0) will fix the undefined val issue, the question is, is it the
> default we want? I haven't worked on MPEG2 (just posted patches
> Ezequiel worked on) so I can't tell.
>
Well, any fields where zero is not a good default, should be assigned here.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: v4l2: Initialize mpeg slice controls
2019-05-29 16:59 ` Ezequiel Garcia
@ 2019-05-29 17:06 ` Boris Brezillon
0 siblings, 0 replies; 7+ messages in thread
From: Boris Brezillon @ 2019-05-29 17:06 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Hans Verkuil, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus, linux-media, kernel
On Wed, 29 May 2019 13:59:50 -0300
Ezequiel Garcia <ezequiel@collabora.com> wrote:
> On Wed, 2019-05-29 at 18:06 +0200, Boris Brezillon wrote:
> > On Wed, 29 May 2019 17:42:58 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > > On 5/29/19 5:36 PM, Ezequiel Garcia wrote:
> > > > On Wed, 2019-05-29 at 16:41 +0200, Hans Verkuil wrote:
> > > > > On 5/3/19 1:42 PM, Boris Brezillon wrote:
> > > > > > Make sure the default value at least passes the std_validate() tests.
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > > ---
> > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 20 +++++++++++++++++++-
> > > > > > 1 file changed, 19 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > > > index b1ae2e555c68..19d40cc6e565 100644
> > > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > > > @@ -1461,7 +1461,14 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> > > > > > static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > > > > > union v4l2_ctrl_ptr ptr)
> > > > > > {
> > > > > > - switch (ctrl->type) {
> > > > > > + struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> > > > > > +
> > > > > > + /*
> > > > > > + * The cast is needed to get rid of a gcc warning complaining that
> > > > > > + * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the
> > > > > > + * v4l2_ctrl_type enum.
> > > > > > + */
> > > > > > + switch ((u32)ctrl->type) {
> > > > > > case V4L2_CTRL_TYPE_STRING:
> > > > > > idx *= ctrl->elem_size;
> > > > > > memset(ptr.p_char + idx, ' ', ctrl->minimum);
> > > > > > @@ -1486,6 +1493,17 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > > > > > case V4L2_CTRL_TYPE_U32:
> > > > > > ptr.p_u32[idx] = ctrl->default_value;
> > > > > > break;
> > > > > > + case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> > > > > > + p_mpeg2_slice_params = ptr.p;
> > > > > > + /* 4:2:0 */
> > > > > > + p_mpeg2_slice_params->sequence.chroma_format = 1;
> > > > > > + /* 8 bits */
> > > > > > + p_mpeg2_slice_params->picture.intra_dc_precision = 0;
> > > > > > + /* interlaced top field */
> > > > > > + p_mpeg2_slice_params->picture.picture_structure = 1;
> > > > > > + p_mpeg2_slice_params->picture.picture_coding_type =
> > > > > > + V4L2_MPEG2_PICTURE_CODING_TYPE_I;
> > > > >
> > > > > Oops, this isn't complete. It should still zero the p_mpeg2_slice_params
> > > > > struct first. Right now any fields not explicitly set just have whatever
> > > > > was in memory.
> >
> > Oops.
> >
> > > > > Can you post a patch fixing this?
> > > > >
> > > > >
> > > >
> > > > I was wondering if we want to zero all the cases, and not just
> > > > the struct types ones.
> > >
> > > The others either overwrite the data with the default_value, or memset
> > > the whole control (default case). It's only for these compound controls
> > > that something special needs to be done.
> > >
> > > The code can be restructured, though: instead of break do return in all
> > > the simple type cases.
> > >
> > > Then call memset followed by a new switch for the compound types where you
> > > need to set some fields to non-zero.
> >
> > memset(0) will fix the undefined val issue, the question is, is it the
> > default we want? I haven't worked on MPEG2 (just posted patches
> > Ezequiel worked on) so I can't tell.
> >
>
> Well, any fields where zero is not a good default, should be assigned here.
>
That's my point: I've only assigned fields that were checked by the core
and where 0 is not a valid value. That doesn't necessarily make 0 a good
default for the other fields :P. Hence my suggestion to have someone
that knows about MPEG2 (you :)) check the other fields.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-29 17:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-03 11:42 [PATCH] media: v4l2: Initialize mpeg slice controls Boris Brezillon
2019-05-29 14:41 ` Hans Verkuil
2019-05-29 15:36 ` Ezequiel Garcia
2019-05-29 15:42 ` Hans Verkuil
2019-05-29 16:06 ` Boris Brezillon
2019-05-29 16:59 ` Ezequiel Garcia
2019-05-29 17:06 ` Boris Brezillon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).