Re: [FFmpeg-devel] [PATCH 5/5] lavfi: make request_frame() non-recursive.
On Tue, Dec 1, 2015 at 11:26 AM, Nicolas George wrote:
> Le decadi 10 frimaire, an CCXXIV, Hendrik Leppkes a écrit :
>> We generally just use ints for boolean properties, any particular
>> reason this uses unsigned instead?
>
> Just a matter of personal habit, justified below. But as I look at it, I see
> it is rather inconsistent with other fields in the struct that are int
> although they really would be better unsigned. Will change.
>
>
> When it does not matter, I usually use unsigned instead of signed because
> that often leaves the compiler more room for optimization. For example,
> "type half(type x) { return x / 2; }" compiles into this for unsigned:
>
> movl%edi, %eax
> shrl%eax
> ret
>
> And this for signed:
>
> movl%edi, %eax
> movl$2, %ecx
> cltd
> idivl %ecx
> ret
>
> In other words, "x / 2" can be optimized into "x >> 1" for unsigned, but not
> for signed because the rounding is not the same for negative.
>
> I suppose in fact you already knew that. Of course it does not make a
> difference here, but I prefer this habit instead of the other way around.
> For booleans, I feel that int gives the impression there is a third case,
> possibly error: think of ssize_t for the return value of syscalls. And if it
> becomes a counter, then it should naturally be unsigned.
All true, but note the bizarre GCC bug I posted regarding perf
regression for unsigned vs signed counters:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48052
May be related to Clement's point below.
>
> Regards,
>
> --
> Nicolas George
>
> ___
> 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
Re: [FFmpeg-devel] [PATCH 5/5] lavfi: make request_frame() non-recursive.
On Tue, Dec 01, 2015 at 05:26:23PM +0100, Nicolas George wrote:
> Le decadi 10 frimaire, an CCXXIV, Hendrik Leppkes a écrit :
> > We generally just use ints for boolean properties, any particular
> > reason this uses unsigned instead?
>
> Just a matter of personal habit, justified below. But as I look at it, I see
> it is rather inconsistent with other fields in the struct that are int
> although they really would be better unsigned. Will change.
>
>
> When it does not matter, I usually use unsigned instead of signed because
> that often leaves the compiler more room for optimization. For example,
> "type half(type x) { return x / 2; }" compiles into this for unsigned:
>
> movl%edi, %eax
> shrl%eax
> ret
>
> And this for signed:
>
> movl%edi, %eax
> movl$2, %ecx
> cltd
> idivl %ecx
> ret
>
> In other words, "x / 2" can be optimized into "x >> 1" for unsigned, but not
> for signed because the rounding is not the same for negative.
>
> I suppose in fact you already knew that. Of course it does not make a
> difference here, but I prefer this habit instead of the other way around.
> For booleans, I feel that int gives the impression there is a third case,
> possibly error: think of ssize_t for the return value of syscalls. And if it
> becomes a counter, then it should naturally be unsigned.
>
There is at least one case where you actually want it signed, which is
when you loop over it. Typically, in the case of a width/height, you
actually want it signed because it indicates the compiler that an overflow
will not happen since a signed overflow is undefined.
for (x = x_start; x < width; x++)
Here, if we are dealing with signed, the compiler can simply assume that x
will not overflow, go back to 0 and then reach width. See
-Wstrict-overflow compiler option.
Regards,
--
Clément B.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 5/5] lavfi: make request_frame() non-recursive.
Le decadi 10 frimaire, an CCXXIV, Hendrik Leppkes a écrit :
> We generally just use ints for boolean properties, any particular
> reason this uses unsigned instead?
Just a matter of personal habit, justified below. But as I look at it, I see
it is rather inconsistent with other fields in the struct that are int
although they really would be better unsigned. Will change.
When it does not matter, I usually use unsigned instead of signed because
that often leaves the compiler more room for optimization. For example,
"type half(type x) { return x / 2; }" compiles into this for unsigned:
movl%edi, %eax
shrl%eax
ret
And this for signed:
movl%edi, %eax
movl$2, %ecx
cltd
idivl %ecx
ret
In other words, "x / 2" can be optimized into "x >> 1" for unsigned, but not
for signed because the rounding is not the same for negative.
I suppose in fact you already knew that. Of course it does not make a
difference here, but I prefer this habit instead of the other way around.
For booleans, I feel that int gives the impression there is a third case,
possibly error: think of ssize_t for the return value of syscalls. And if it
becomes a counter, then it should naturally be unsigned.
Regards,
--
Nicolas George
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 5/5] lavfi: make request_frame() non-recursive.
On Sun, Nov 29, 2015 at 5:21 PM, Nicolas George wrote:
> Instead of calling the input filter request_frame() method,
> ff_request_frame() now marks the link and returns immediately.
> bufferskin is changed to activate the marked filters until
> a frame is obtained.
>
> Signed-off-by: Nicolas George
> ---
> libavfilter/avfilter.c | 20 +++-
> libavfilter/avfilter.h | 14 +++
> libavfilter/avfiltergraph.c | 58
> +
> libavfilter/buffersink.c| 5
> libavfilter/internal.h | 7 ++
> 5 files changed, 99 insertions(+), 5 deletions(-)
>
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index 9c7b462..e413546 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -185,6 +185,7 @@ void ff_avfilter_link_set_in_status(AVFilterLink *link,
> int status, int64_t pts)
> void ff_avfilter_link_set_out_status(AVFilterLink *link, int status, int64_t
> pts)
> {
> link->status = status;
> +link->frame_wanted_in = link->frame_wanted_out = 0;
> ff_update_link_current_pts(link, pts);
> }
>
> @@ -353,11 +354,21 @@ void ff_tlog_link(void *ctx, AVFilterLink *link, int
> end)
>
> int ff_request_frame(AVFilterLink *link)
> {
> -int ret = -1;
> FF_TPRINTF_START(NULL, request_frame); ff_tlog_link(NULL, link, 1);
>
> if (link->status)
> return link->status;
> +link->frame_wanted_in = 1;
> +link->frame_wanted_out = 1;
> +return 0;
> +}
> +
> +int ff_request_frame_to_filter(AVFilterLink *link)
> +{
> +int ret = -1;
> +
> +FF_TPRINTF_START(NULL, request_frame_to_filter); ff_tlog_link(NULL,
> link, 1);
> +link->frame_wanted_in = 0;
> if (link->srcpad->request_frame)
> ret = link->srcpad->request_frame(link);
> else if (link->src->inputs[0])
> @@ -366,6 +377,9 @@ int ff_request_frame(AVFilterLink *link)
> AVFrame *pbuf = link->partial_buf;
> link->partial_buf = NULL;
> ret = ff_filter_frame_framed(link, pbuf);
> +ff_avfilter_link_set_in_status(link, AVERROR_EOF, AV_NOPTS_VALUE);
> +link->frame_wanted_out = 0;
> +return ret;
> }
> if (ret < 0) {
> if (ret != AVERROR(EAGAIN) && ret != link->status)
> @@ -1135,6 +1149,9 @@ static int ff_filter_frame_needs_framing(AVFilterLink
> *link, AVFrame *frame)
> if (pbuf->nb_samples >= link->min_samples) {
> ret = ff_filter_frame_framed(link, pbuf);
> pbuf = NULL;
> +} else {
> +if (link->frame_wanted_out)
> +link->frame_wanted_in = 1;
> }
> }
> av_frame_free(&frame);
> @@ -1176,6 +1193,7 @@ int ff_filter_frame(AVFilterLink *link, AVFrame *frame)
> }
> }
>
> +link->frame_wanted_out = 0;
> /* Go directly to actual filtering if possible */
> if (link->type == AVMEDIA_TYPE_AUDIO &&
> link->min_samples &&
> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> index f3a0f2d..801f29d 100644
> --- a/libavfilter/avfilter.h
> +++ b/libavfilter/avfilter.h
> @@ -515,6 +515,20 @@ struct AVFilterLink {
> * Number of past frames sent through the link.
> */
> int64_t frame_count;
> +
> +/**
> + * True if a frame is currently wanted on the input of this filter.
> + * Set when ff_request_frame() is called by the output,
> + * cleared when the request is handled or forwarded.
> + */
> +unsigned frame_wanted_in;
> +
> +/**
> + * True if a frame is currently wanted on the output of this filter.
> + * Set when ff_request_frame() is called by the output,
> + * cleared when a frame is filtered.
> + */
> +unsigned frame_wanted_out;
We generally just use ints for boolean properties, any particular
reason this uses unsigned instead?
> };
>
> /**
> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> index ec2245f..9f50b41 100644
> --- a/libavfilter/avfiltergraph.c
> +++ b/libavfilter/avfiltergraph.c
> @@ -1367,11 +1367,14 @@ void ff_avfilter_graph_update_heap(AVFilterGraph
> *graph, AVFilterLink *link)
>
> int avfilter_graph_request_oldest(AVFilterGraph *graph)
> {
> +AVFilterLink *oldest = graph->sink_links[0];
> +int r;
> +
> while (graph->sink_links_count) {
> -AVFilterLink *oldest = graph->sink_links[0];
> -int r = ff_request_frame(oldest);
> +oldest = graph->sink_links[0];
> +r = ff_request_frame(oldest);
> if (r != AVERROR_EOF)
> -return r;
> +break;
> av_log(oldest->dst, AV_LOG_DEBUG, "EOF on sink link %s:%s.\n",
> oldest->dst ? oldest->dst->name : "unknown",
> oldest->dstpad ? oldest->dstpad->name : "unknown");
> @@ -1381,5 +1384,52 @@ int avfilter_graph_request_oldest(AVFilterGraph *graph)
> oldest->age_index);
> oldest->age_index = -1;
> }
> -retu
Re: [FFmpeg-devel] [PATCH 5/5] lavfi: make request_frame() non-recursive.
On Sun, Nov 29, 2015 at 05:21:52PM +0100, Nicolas George wrote: > Instead of calling the input filter request_frame() method, > ff_request_frame() now marks the link and returns immediately. > bufferskin is changed to activate the marked filters until sink didnt review the rest (might do it later if noone else does but thats no objection if you feel this is fine and want to push it) thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
