[FFmpeg-devel] [PATCH] Added QSV based VPP filter - second try

[Moritz Barsnick]

On Thu, Nov 05, 2015 at 11:18:38 +0100, Sven Dueking wrote:

+The VPP library is part of the Intel® Media SDK and exposes the media acceleration capabilities of Intel® platforms.

You're probably supposed to limit the line length in the free text to
some width, but I don't know the style guide for this. (It doesn't
matter for the documents' representation.)

+So user can select between speed and quality
Thus the user can choose between speed and quality.
("So" sounds peculiar. "User" requires the article "the". It's more a
choice than a selection, so "choose". And a period at the end of the
sentence.)

+ at item Output video width (Range [32,4096]).
"range" instead of "Range".

+ at item Output video height (Range [32,2304]).
ditto

+Sets output frame rate (frames/ seconds (fps)).
Apart from the stray whitespace around '/', I'm not sure that's the
normal way to express it.

+Value is used.

"Given value is used"?

+ at item Specifies how many asynchronous operations VPP performs before the results are explicitly synchronized (Default Value is 4).
"Value" -> "value".

+ at item Specifies how many B frames are used, this value sets the number of extra surfaces used for re-ordering (Default Value is 3).
ditto
And make two sentences of it or use a semicolon instead of a comma.

+Wrong configuration could result in lost frames, since the VPP works asynchronous and could request multiple surfaces.

"asynchronously" (it's an adverb here, referring to the verb "works").

+ at emph{Frame size}: frame width must be
+a multiple of 16, frame height must be a multiple of 16 for progressive frames and a multiple
+of 32 otherwise.

Is this enforced by the filter? (Just asking - too lazy to check
anyway. ;-))

+ * This file is part of FFmpeg.
+ * Libav is free software; you can redistribute it and/or

All the files I found referring to being part of FFmpeg also continue
with "FFmpeg is free software", not "Libav is free software".

+/**
+ * ToDo :
+ *
+ * - cropping
+ */

Does this remark belong in the source? (I frankly don't know - probably
OK.)

+    { "h",           "Output video height",                                    OFFSET(out_height),   AV_OPT_TYPE_INT, {.i64=0}, 0, 2304, .flags = FLAGS },
+    { "height",      "Output video height : ",                                 OFFSET(out_height),   AV_OPT_TYPE_INT, {.i64=0}, 0, 2304, .flags = FLAGS },

Shouldn't these have the identical description string?

+    { "max_b_frames","Maximum number of b frames  [default = 3]",              OFFSET(max_b_frames), AV_OPT_TYPE_INT, { .i64 = 3 }, 0, INT_MAX, .flags = FLAGS },
Missing space after comma, stray whitespace in "frames  ["

+        memset(vpp->out_surface[i], 0, sizeof(mfxFrameSurface1));
Didn't you introduce a macro for memsetting to zero?

+        memset(&vpp->deinterlace_conf, 0, sizeof(mfxExtVPPDeinterlacing));
ditto (and more of these). Or just drop the macro.


Gruß,
Moritz