> -----Ursprüngliche Nachricht-----
> Von: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] Im Auftrag
> von Moritz Barsnick
> Gesendet: Donnerstag, 5. November 2015 12:01
> An: FFmpeg development discussions and patches
> Betreff: Re: [FFmpeg-devel] [PATCH] Added QSV based VPP filter - second
> try
>
> 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.)
>
> +@item Output video width (Range [32,4096]).
> "range" instead of "Range".
>
> +@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.
Mh, I guess "Sets output frame rate" should be enough, do you agree ?
>
> +Value is used.
>
> "Given value is used"?
>
> +@item Specifies how many asynchronous operations VPP performs before
> the results are explicitly synchronized (Default Value is 4).
> "Value" -> "value".
>
> +@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").
>
> +@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.)
Not sure, I can remove these lines - just a comment to mark this filter as
"under development" ;).
>
> + { "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?
I think so, thanks.
>
> + { "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.
Ok, will think about this.
>
>
> Gruß,
> Moritz
Thanks for your review !
Will wait for more feedback and commit a new patch tomorrow.
Gruß zurück,
Sven
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
