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

[Sven Dueking]

> -----Ursprüngliche Nachricht-----
> Von: ffmpeg-devel [mailto:ffmpeg-devel-bounces at 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.)
> 
> + 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.

Mh, I guess "Sets output frame rate" should be enough, do you agree ? 

> 
> +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.)

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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel