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> </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-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
