Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RFC] Enhance message content send to 'stderr' #9

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

[RFC] Enhance message content send to 'stderr' #9

wants to merge 7 commits into from

Conversation

rzerres
Copy link
Contributor

@rzerres rzerres commented Mar 6, 2019 •
edited
Loading

v1:

  • recursive use of config->quiet flag
    append new argument to function calls. This can be evaluated to post enhanced messages if not (!quiet) selected
  • update for config options
    define a resonable default slice_path in setup-config()

@rzerres
v4l2-request-test.c: introduce parsing for 'long options'
53355bb 
- enable getopt_long supporting long option style
  (e.g --video-path, --fps, etc.)
- try to be consistent which 'v4l2-compliance' binary when
  choosing option names
- adapt help message
- to adopt the slice-path you need to explicitly prepend an
  argument-option (-s or --slice-path) with the path as an option-string
- adopt summary output to be in line with long options
- typo corrections
- update code style (tabify vs whitespace, alingement)

Signed-off-by: Ralf Zerres <[email protected]>
@rzerres
Merge branch 'master' of https://github.com/bootlin/v4l2-request-test …
e26d443 
…into HEAD
@rzerres rzerres force-pushed the wip-verbose branch 3 times, most recently from 8735d3e to 07de388 Compare March 7, 2019 02:40
paulkocialkowski
paulkocialkowski reviewed Mar 7, 2019
View reviewed changes
v4l2-request-test.c
@@ -122,20 +122,17 @@ static void print_summary(struct config *config, struct preset *preset)
printf(" Height: %d\n", preset->height);
printf(" Frames count: %d\n", preset->frames_count);

printf(" Format: ");
printf(" Codec Type: ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing Format with Codec is definitely a change I would take in, but adding more than the short names for the codecs seems overkill. If you'd like to improve this area, one thing would be to use proper names: MPEG-2, H.264 and H.265 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will produce an update.

v4l2.c
/* Ref: https://www.kernel.org/doc/html/v4.10/media/uapi/v4l/vidioc-g-ext-ctrls.html */
fprintf(stderr, "Initialized 'controls' structure (ctrl_class: %d, which: %d)\n",
controls.ctrl_class, controls.which);
fprintf(stderr, " 'control' value (id: %d, size: %d)\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That feels overkill, I don't think I'll take it in, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep it in my branch. For testing

v4l2.c
control.id, control.size);

rc = ioctl(video_fd, VIDIOC_S_EXT_CTRLS, &controls);
if (rc < 0) {
fprintf(stderr, "Unable to set control: %s\n", strerror(errno));
fprintf(stderr, "Unable to set control: %s (%d)\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one is a keeper!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will create an seperate PR

v4l2.c
@@ -597,6 +601,9 @@ bool video_engine_capabilities_test(int video_fd,
unsigned int capabilities;
int rc;

if(!quiet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the changes below feel overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep it in my brach. Just for testing ....

v4l2-request-test.c Outdated
@@ -217,6 +217,7 @@ static void setup_config(struct config *config)
config->drm_driver = strdup("sun4i-drm");

config->preset_name = strdup("bbb-mpeg2");
asprintf(&config->slices_path, "data/%s", config->preset_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep this in main() so that we alloc once instead of alloc + free + alloc when user specifies another path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. [v5] update already send.

v4l2.c
@@ -509,7 +515,7 @@ static int set_format_controls(int video_fd, int request_fd,
V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX,
&frame->h264.scaling_matrix,
sizeof(frame->h264.scaling_matrix) },
{ CODEC_TYPE_H264, "scaling matrix",
{ CODEC_TYPE_H264, "slice parameters",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, feel free to spin this as a standalone fix.

drm.c
@@ -649,7 +649,8 @@ int display_engine_start(int drm_fd, unsigned int width, unsigned int height,
struct format_description *format,
struct video_buffer *video_buffers, unsigned int count,
struct gem_buffer **buffers,
struct display_setup *setup)
struct display_setup *setup,
bool quiet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions are not expected to interact with the user other than for printing out errors, so I don't think I'll take this in.

@rzerres
introduce 'codec' structure (with description)
52ff3d1 
- const struct codec (.name, .description, .type)
  use more descriptive names when referencing codecs by enum type

Signed-off-by: Ralf Zerres <[email protected]>
@rzerres
recursive use of config->quiet flag
d069211 
- respect quiet field in function calls
- otherwise show descriptive control messages

Signed-off-by: Ralf Zerres <[email protected]>
@rzerres
introduce verbose messages
a26d9de 
- get more descriptive response messages on stderr

Signed-off-by: Ralf Zerres <[email protected]>
@rzerres
Comment out project's local header for H.264 and HEVC
437da0b 
- h264-ctrls.h
- hevc-ctrls.h

compile will rely on up to date system/uapi headers

Signed-off-by: Ralf Zerres <[email protected]>
@rzerres
Include project's local headers for H.264 and HEVC
a55d409 
- h264-ctrls.h
- hevc-ctrls.h

Signed-off-by: Ralf Zerres <[email protected]>
@rzerres
Copy link
Contributor Author

rzerres commented Mar 8, 2019

This discussion can be closed.
Your comments are embedded in the new PR's

It took me a while to understand, that the github GUI requires an author to group disired commits for new PR's in its own branch. You aren't able to 'exclude' or 'include' just single commit hashes from a single working branch into the PR. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulkocialkowski paulkocialkowski paulkocialkowski left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rzerres @paulkocialkowski