Re: [PATCH] CHROMIUM: s5p-mfc: add controls to set vp8 enc profile

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

Thanks for the review.

On Mon, Dec 9, 2013 at 6:52 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> Hi Arun,
>
> Some comments below...
>
> On 12/09/2013 02:16 PM, Arun Kumar K wrote:
>> Add v4l2 controls to set desired profile for VP8 encoder.
>> Acceptable levels for VP8 encoder are
>> 0: Version 0
>> 1: Version 1
>> 2: Version 2
>> 3: Version 3
>>
>> Signed-off-by: Pawel Osciak <posciak@xxxxxxxxxxxx>
>> Signed-off-by: Kiran AVND <avnd.kiran@xxxxxxxxxxx>
>> Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx>
>> ---
>> This patch is rebased over another VP8 control patch from me:
>> https://linuxtv.org/patch/20733/
>> ---
>>  Documentation/DocBook/media/v4l/controls.xml    |    9 +++++++++
>>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h |    1 +
>>  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    |   11 +++++++++++
>>  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |    6 ++----
>>  drivers/media/v4l2-core/v4l2-ctrls.c            |    8 ++++++++
>>  include/uapi/linux/v4l2-controls.h              |    1 +
>>  6 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
>> index e4db4ac..c1f7544 100644
>> --- a/Documentation/DocBook/media/v4l/controls.xml
>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>> @@ -3193,6 +3193,15 @@ V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame.</entry>
>>             <row><entry spanname="descr">Quantization parameter for a P frame for VP8.</entry>
>>             </row>
>>
>> +           <row><entry></entry></row>
>> +           <row>
>> +             <entry spanname="id"><constant>V4L2_CID_MPEG_VIDEO_VPX_PROFILE</constant>&nbsp;</entry>
>> +             <entry>integer</entry>
>
> This says 'integer' whereas the control is actually an integer menu.
>
> Why did you choose 'integer menu' for this? Would a regular integer or perhaps a standard
> menu be better?
>

I chose integer menu as it is standard set of values with only 4
options (integers).
Same thing is done in the controls - V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS
and V4L2_CID_MPEG_VIDEO_VPX_NUM_REF_FRAMES. I felt this new controls is also
in-line with the requirement of a integer-menu type. What do you think?

>> +           </row>
>> +           <row><entry spanname="descr">Select the desired profile for VP8 encoder.
>> +Acceptable values are 0, 1, 2 and 3 corresponding to encoder versions 0, 1, 2 and 3.</entry>
>
> Is it a 'profile' or a 'version'? It looks a bit confusing. I don't have the VP8 standard,
> so I can't really tell what the correct terminology is.
>

Ok will make it more clear.

> Also, does this control apply just to VP8 or also to other VP versions? The control name
> says 'VPX' while the description says 'VP8' explicitly.
>

As of now its applicable to VP8, but I am not sure if the same thing
will apply to VP9 also.

Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux