[FFmpeg-devel,1/2] avfilter: change ff_inlink_make_frame_writable() to take AVFrame* argument

Submitted by Muhammad Faiz on Jan. 28, 2017, 3:23 p.m.

Details

Message ID 1485617034-31431-1-git-send-email-mfcc64@gmail.com
State New
Headers show

Commit Message

Muhammad Faiz Jan. 28, 2017, 3:23 p.m.
so the behavior will be similar to
av_frame_make_writable().

Also use av_frame_copy() replacing
av_image_copy()/av_samples_copy().

Needed for the next patch.

Suggested-by: wm4 <nfxjfg@googlemail.com>
Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
---
 libavfilter/avfilter.c | 26 ++++++--------------------
 libavfilter/filters.h  |  2 +-
 2 files changed, 7 insertions(+), 21 deletions(-)

Comments

wm4 Jan. 28, 2017, 3:37 p.m.
On Sat, 28 Jan 2017 22:23:53 +0700
Muhammad Faiz <mfcc64@gmail.com> wrote:

> so the behavior will be similar to
> av_frame_make_writable().
> 
> Also use av_frame_copy() replacing
> av_image_copy()/av_samples_copy().
> 
> Needed for the next patch.
> 
> Suggested-by: wm4 <nfxjfg@googlemail.com>
> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
> ---
>  libavfilter/avfilter.c | 26 ++++++--------------------
>  libavfilter/filters.h  |  2 +-
>  2 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index c12d491..c8dafd2 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -1104,7 +1104,7 @@ static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
>          filter_frame = default_filter_frame;
>  
>      if (dst->needs_writable) {
> -        ret = ff_inlink_make_frame_writable(link, &frame);
> +        ret = ff_inlink_make_frame_writable(link, frame);
>          if (ret < 0)
>              goto fail;
>      }
> @@ -1556,9 +1556,8 @@ int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
>      return 1;
>  }
>  
> -int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe)
> +int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame *frame)
>  {
> -    AVFrame *frame = *rframe;
>      AVFrame *out;
>      int ret;
>  
> @@ -1585,23 +1584,10 @@ int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe)
>          return ret;
>      }
>  
> -    switch (link->type) {
> -    case AVMEDIA_TYPE_VIDEO:
> -        av_image_copy(out->data, out->linesize, (const uint8_t **)frame->data, frame->linesize,
> -                      frame->format, frame->width, frame->height);
> -        break;
> -    case AVMEDIA_TYPE_AUDIO:
> -        av_samples_copy(out->extended_data, frame->extended_data,
> -                        0, 0, frame->nb_samples,
> -                        av_frame_get_channels(frame),
> -                        frame->format);
> -        break;
> -    default:
> -        av_assert0(!"reached");
> -    }
> -
> -    av_frame_free(&frame);
> -    *rframe = out;
> +    av_frame_copy(out, frame);
> +    av_frame_unref(frame);
> +    av_frame_move_ref(frame, out);
> +    av_frame_free(&out);
>      return 0;
>  }
>  
> diff --git a/libavfilter/filters.h b/libavfilter/filters.h
> index 2c78d60..5d32403 100644
> --- a/libavfilter/filters.h
> +++ b/libavfilter/filters.h
> @@ -101,7 +101,7 @@ int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
>   * This is similar to av_frame_make_writable() except it uses the link's
>   * buffer allocation callback, and therefore allows direct rendering.
>   */
> -int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe);
> +int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame *frame);
>  
>  /**
>   * Test and acknowledge the change of status on the link.

LGTM. Maybe you should wait for confirmation by Nicolas George or
someone else who knows this code well before you push this.
Nicolas George Jan. 28, 2017, 3:43 p.m.
Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :
> so the behavior will be similar to
> av_frame_make_writable().

The point was to move away from that. Who copies a struct when you can
move a pointer?

Regards,
wm4 Jan. 28, 2017, 3:57 p.m.
On Sat, 28 Jan 2017 16:43:36 +0100
Nicolas George <george@nsup.org> wrote:

> Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :
> > so the behavior will be similar to
> > av_frame_make_writable().  
> 
> The point was to move away from that. Who copies a struct when you can
> move a pointer?
> 
> Regards,
> 

Because it's trickier, harder to use, and creates the risk of dangling
pointers that will be spotted only if a frame was not writable.
Muhammad Faiz Jan. 28, 2017, 4:23 p.m.
On 1/28/17, Nicolas George <george@nsup.org> wrote:
> Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :
>> so the behavior will be similar to
>> av_frame_make_writable().
>
> The point was to move away from that. Who copies a struct when you can
> move a pointer?

So, if ff_inlink_make_frame_writable() takes AVFrame**, it will be
incompatible with framequeue framework, because it stores AVFrame*, so
it will contain dangling AVFrame*.

So, which one you choose? This patch or the previous patch that makes
frame writable manually?

Thank's
Nicolas George Jan. 28, 2017, 5:35 p.m.
Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :
> So, if ff_inlink_make_frame_writable() takes AVFrame**, it will be
> incompatible with framequeue framework, because it stores AVFrame*, so
> it will contain dangling AVFrame*.

There is nothing "incompatible" about it, what would it even mean? It
just can not be used as is on this specific call site; it was not meant
to. It is a smaller, more versatile brick. You just need to add the
little bit of mortar to have it fit here.

It is not possible to "make" a frame writable, the original name is a
misnomer; the only thing possible is to create a new copy that is
uniquely referenced and therefore writable. If that is needed, an extra
copy can be made to have the new frame take the place of the old one.
But that is only rarely needed; making that copy always like you propose
following wm4's bad advice will just make the code less efficient and
more awkward.

Regards,
wm4 Jan. 28, 2017, 6:02 p.m.
On Sat, 28 Jan 2017 18:35:01 +0100
Nicolas George <george@nsup.org> wrote:

> Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :
> > So, if ff_inlink_make_frame_writable() takes AVFrame**, it will be
> > incompatible with framequeue framework, because it stores AVFrame*, so
> > it will contain dangling AVFrame*.  
> 
> There is nothing "incompatible" about it, what would it even mean? It
> just can not be used as is on this specific call site; it was not meant
> to. It is a smaller, more versatile brick. You just need to add the
> little bit of mortar to have it fit here.
> 
> It is not possible to "make" a frame writable, the original name is a
> misnomer; the only thing possible is to create a new copy that is

Well, that's what the existing API calls it. In several places.

> uniquely referenced and therefore writable. If that is needed, an extra
> copy can be made to have the new frame take the place of the old one.
> But that is only rarely needed; making that copy always like you propose

It doesn't always copy.

> following wm4's bad advice

Please cease your childish attacks.

> will just make the code less efficient and
> more awkward.

Amazing.

We're talking about copying a few bytes additional in a case that
copies an entire frame of media data. And it's only "rarely needed", as
you said just above. (To remind other readers,
ff_inlink_make_frame_writable() exits immediately if the frame is
writable.)
wm4 Jan. 29, 2017, 11:44 a.m.
On Sat, 28 Jan 2017 22:23:53 +0700
Muhammad Faiz <mfcc64@gmail.com> wrote:

> so the behavior will be similar to
> av_frame_make_writable().
> 
> Also use av_frame_copy() replacing
> av_image_copy()/av_samples_copy().
> 
> Needed for the next patch.
> 
> Suggested-by: wm4 <nfxjfg@googlemail.com>
> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
> ---
>  libavfilter/avfilter.c | 26 ++++++--------------------
>  libavfilter/filters.h  |  2 +-
>  2 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index c12d491..c8dafd2 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -1104,7 +1104,7 @@ static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
>          filter_frame = default_filter_frame;
>  
>      if (dst->needs_writable) {
> -        ret = ff_inlink_make_frame_writable(link, &frame);
> +        ret = ff_inlink_make_frame_writable(link, frame);
>          if (ret < 0)
>              goto fail;
>      }
> @@ -1556,9 +1556,8 @@ int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
>      return 1;
>  }
>  
> -int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe)
> +int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame *frame)
>  {
> -    AVFrame *frame = *rframe;
>      AVFrame *out;
>      int ret;
>  
> @@ -1585,23 +1584,10 @@ int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe)
>          return ret;
>      }
>  
> -    switch (link->type) {
> -    case AVMEDIA_TYPE_VIDEO:
> -        av_image_copy(out->data, out->linesize, (const uint8_t **)frame->data, frame->linesize,
> -                      frame->format, frame->width, frame->height);
> -        break;
> -    case AVMEDIA_TYPE_AUDIO:
> -        av_samples_copy(out->extended_data, frame->extended_data,
> -                        0, 0, frame->nb_samples,
> -                        av_frame_get_channels(frame),
> -                        frame->format);
> -        break;
> -    default:
> -        av_assert0(!"reached");
> -    }
> -
> -    av_frame_free(&frame);
> -    *rframe = out;
> +    av_frame_copy(out, frame);
> +    av_frame_unref(frame);
> +    av_frame_move_ref(frame, out);
> +    av_frame_free(&out);
>      return 0;
>  }
>  
> diff --git a/libavfilter/filters.h b/libavfilter/filters.h
> index 2c78d60..5d32403 100644
> --- a/libavfilter/filters.h
> +++ b/libavfilter/filters.h
> @@ -101,7 +101,7 @@ int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
>   * This is similar to av_frame_make_writable() except it uses the link's
>   * buffer allocation callback, and therefore allows direct rendering.
>   */
> -int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe);
> +int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame *frame);
>  
>  /**
>   * Test and acknowledge the change of status on the link.

Please push this patch. It's a nice simplification.
Nicolas George Jan. 29, 2017, 11:47 a.m.
Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> Please push this patch. It's a nice simplification.

I rejected this patch. This message is unacceptable.
wm4 Jan. 29, 2017, 11:52 a.m.
On Sun, 29 Jan 2017 12:47:16 +0100
Nicolas George <george@nsup.org> wrote:

> Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> > Please push this patch. It's a nice simplification.  
> 
> I rejected this patch. This message is unacceptable.

You hadn't anything to say my convincing arguments.

His 1/2 patch was a strict improvement over the current internal API.
In particular, it gets it into line with the current
av_frame_make_writable() API. There are other good arguments. Why
reject it? Because you're so stubborn?
Nicolas George Jan. 29, 2017, 11:54 a.m.
Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> You hadn't anything to say my convincing arguments.
> 
> His 1/2 patch was a strict improvement over the current internal API.
> In particular, it gets it into line with the current
> av_frame_make_writable() API. There are other good arguments. Why
> reject it? Because you're so stubborn?

I will not answer the contents of your messages unless you start
addressing me and the other persons on the list politely.
wm4 Jan. 29, 2017, 11:57 a.m.
On Sun, 29 Jan 2017 12:54:53 +0100
Nicolas George <george@nsup.org> wrote:

> Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> > You hadn't anything to say my convincing arguments.
> > 
> > His 1/2 patch was a strict improvement over the current internal API.
> > In particular, it gets it into line with the current
> > av_frame_make_writable() API. There are other good arguments. Why
> > reject it? Because you're so stubborn?  
> 
> I will not answer the contents of your messages unless you start
> addressing me and the other persons on the list politely.

OK:

His 1/2 patch was a strict improvement over the current internal API.
In particular, it gets it into line with the current
av_frame_make_writable() API. There are other good arguments. Why
reject it?
Nicolas George Jan. 29, 2017, noon
Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> His 1/2 patch was a strict improvement over the current internal API.
> In particular, it gets it into line with the current
> av_frame_make_writable() API.

You purposefully ignore the fact, already stated, that moving away from
av_frame_make_writable() was intentional.
wm4 Jan. 29, 2017, 12:04 p.m.
On Sun, 29 Jan 2017 13:00:38 +0100
Nicolas George <george@nsup.org> wrote:

> Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> > His 1/2 patch was a strict improvement over the current internal API.
> > In particular, it gets it into line with the current
> > av_frame_make_writable() API.  
> 
> You purposefully ignore the fact, already stated, that moving away from
> av_frame_make_writable() was intentional.

Then  you should stop moving away from it. The av_frame_make_writable
design is superior for multiple reasons, and was intentionally designed
this way by an intelligent mind with a lot of API-foresight.

If you "move away from it" just like this, you create a major
inconsistency as well.

I haven't heard a good argument as to why its API is supposed to be
better than av_frame_make_writable.

I one the other hand delivered a bunch of arguments to which you didn't
reply.
Nicolas George Jan. 29, 2017, 12:06 p.m.
Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> Then  you should stop moving away from it. The av_frame_make_writable
> design is superior for multiple reasons, and was intentionally designed
> this way by an intelligent mind with a lot of API-foresight.
> 
> If you "move away from it" just like this, you create a major
> inconsistency as well.
> 
> I haven't heard a good argument as to why its API is supposed to be
> better than av_frame_make_writable.
> 
> I one the other hand delivered a bunch of arguments to which you didn't
> reply.

Rude again.
wm4 Jan. 29, 2017, 12:09 p.m.
On Sun, 29 Jan 2017 13:06:13 +0100
Nicolas George <george@nsup.org> wrote:

> Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> > Then  you should stop moving away from it. The av_frame_make_writable
> > design is superior for multiple reasons, and was intentionally designed
> > this way by an intelligent mind with a lot of API-foresight.
> > 
> > If you "move away from it" just like this, you create a major
> > inconsistency as well.
> > 
> > I haven't heard a good argument as to why its API is supposed to be
> > better than av_frame_make_writable.
> > 
> > I one the other hand delivered a bunch of arguments to which you didn't
> > reply.  
> 
> Rude again.

No, it's not rude.

Maybe you could get technical again, after you've sent only a bunch of
cranky posts complaining about my supposedly bad behavior?
Paul B Mahol Jan. 29, 2017, 12:20 p.m.
On 1/29/17, wm4 <nfxjfg@googlemail.com> wrote:
> On Sun, 29 Jan 2017 13:06:13 +0100
> Nicolas George <george@nsup.org> wrote:
>
>> Le decadi 10 pluviose, an CCXXV, wm4 a ecrit :
>> > Then  you should stop moving away from it. The av_frame_make_writable
>> > design is superior for multiple reasons, and was intentionally designed
>> > this way by an intelligent mind with a lot of API-foresight.
>> >
>> > If you "move away from it" just like this, you create a major
>> > inconsistency as well.
>> >
>> > I haven't heard a good argument as to why its API is supposed to be
>> > better than av_frame_make_writable.
>> >
>> > I one the other hand delivered a bunch of arguments to which you didn't
>> > reply.
>>
>> Rude again.
>
> No, it's not rude.
>
> Maybe you could get technical again, after you've sent only a bunch of
> cranky posts complaining about my supposedly bad behavior?

I'm really sick of you two.
wm4 Jan. 29, 2017, 12:21 p.m.
On Sun, 29 Jan 2017 13:06:13 +0100
Nicolas George <george@nsup.org> wrote:

> Le decadi 10 pluviôse, an CCXXV, wm4 a écrit :
> > Then  you should stop moving away from it. The av_frame_make_writable
> > design is superior for multiple reasons, and was intentionally designed
> > this way by an intelligent mind with a lot of API-foresight.
> > 
> > If you "move away from it" just like this, you create a major
> > inconsistency as well.
> > 
> > I haven't heard a good argument as to why its API is supposed to be
> > better than av_frame_make_writable.
> > 
> > I one the other hand delivered a bunch of arguments to which you didn't
> > reply.  
> 
> Rude again.

Anyway, since you're unwilling to discuss the technical points of this
patch, and seeing you're not maintainer of libavfilter or this file, I
will push patch 1/2 tomorrow.
wm4 Jan. 29, 2017, 12:26 p.m.
On Sun, 29 Jan 2017 13:20:29 +0100
Paul B Mahol <onemda@gmail.com> wrote:

> On 1/29/17, wm4 <nfxjfg@googlemail.com> wrote:
> > On Sun, 29 Jan 2017 13:06:13 +0100
> > Nicolas George <george@nsup.org> wrote:
> >  
> >> Le decadi 10 pluviose, an CCXXV, wm4 a ecrit :  
> >> > Then  you should stop moving away from it. The av_frame_make_writable
> >> > design is superior for multiple reasons, and was intentionally designed
> >> > this way by an intelligent mind with a lot of API-foresight.
> >> >
> >> > If you "move away from it" just like this, you create a major
> >> > inconsistency as well.
> >> >
> >> > I haven't heard a good argument as to why its API is supposed to be
> >> > better than av_frame_make_writable.
> >> >
> >> > I one the other hand delivered a bunch of arguments to which you didn't
> >> > reply.  
> >>
> >> Rude again.  
> >
> > No, it's not rude.
> >
> > Maybe you could get technical again, after you've sent only a bunch of
> > cranky posts complaining about my supposedly bad behavior?  
> 
> I'm really sick of you two.

I too would rather prefer having a technical discussion instead of
shit-flinging. But that is apparently not possible.
Michael Niedermayer Jan. 29, 2017, 4:29 p.m.
On Sat, Jan 28, 2017 at 10:23:53PM +0700, Muhammad Faiz wrote:
> so the behavior will be similar to
> av_frame_make_writable().
> 
> Also use av_frame_copy() replacing
> av_image_copy()/av_samples_copy().
> 
> Needed for the next patch.
> 
> Suggested-by: wm4 <nfxjfg@googlemail.com>
> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
> ---
>  libavfilter/avfilter.c | 26 ++++++--------------------
>  libavfilter/filters.h  |  2 +-
>  2 files changed, 7 insertions(+), 21 deletions(-)

It seems this thread escalated a bit, iam replying here to the root
mail which is unrelated and a reply to a unrelated developer as i do
not want to take a side nor do i have any oppinion about the code
change suggested.

But id like to ask everyone to NOT escalate this further, iam
sure that nothing good would come out of that. No matter if it would
end in someone leaving or some votes which on all outcomes do us harm

also we need a maintainer for the libavfilter core or a area covering
avfilter.c and that person should then make a decission.

ANY decission is better than a spiral of escalation. I mean, lets be
honest neither solution for this really has a problem, they are all
fine, but a fight that escalates and causes temporary or worse
permanant alienation is really bad

I guess my mail got longer than intended, just wanted to basically
say love each other dont hate and fight at least not to the point were
it turns harmfull

[...]
Reto Kromer Jan. 29, 2017, 5:03 p.m.
Michael Niedermayer wrote:

>But id like to ask everyone to NOT escalate this further,
>iam sure that nothing good would come out of that.

+11111111
Nicolas George Jan. 29, 2017, 6:43 p.m.
Le decadi 10 pluviôse, an CCXXV, Michael Niedermayer a écrit :
> also we need a maintainer for the libavfilter core or a area covering
> avfilter.c and that person should then make a decission.

I have more or less acted as the de-facto maintainer of all that has to
do with scheduling in the lavfi framework. I can take over the rest of
the work (that would have required, for example, giving more attention
to the frame pool patches) if people want. I intended to propose when my
work of overhauling the design would have been finished.

I will not propose it officially myself right now, especially not as a
patch for the MAINTAINERS file, as it would look like escalating the
game of core-wars. But if somebody else pushes a corresponding patch I
will not object at all.

> But id like to ask everyone to NOT escalate this further, iam
> sure that nothing good would come out of that.

I fully agree.

wm4 has threatened to push a patch that I have explicitly and
unambiguously rejected. Furthermore, Muhammad, the original author of
this patch, has approved an alternate solution for fixing the same
issue.

Under these circumstances, pushing the patch would be a deliberate act
of escalation.

Note: I rejected the patch based on the main change it advertises, the
change in the function interface. The patch also contains changes making
the implementation simpler. These change are very worthy. If Muhammad
proposes them separately I will gladly approve and apply; and if not I
will eventually do it myself.

>							       as i do
> not want to take a side

When witnessing a bullying situation, not taking a side amounts to
siding with the bully. I will leave the readers decide for themselves if
and how that statement applies to the current situation.

As for myself, I do not wish to have any further interactions at all
with wm4. Starting now, as far as I am concerned, for all intents and
purposes, they and their messages no longer exist. I will reconsider my
position if I learn they can go for a year without badmouthing the
project or mocking or disparaging any of its contributors.

Of course, that means that all their proposals on areas of code for
which I am responsible are silently rejected. If somebody else sponsors
such a change, I will discuss it as if it were their own with all the
good-will that I am capable of.

If another member of the project considers this stance unacceptable, you
can take your responsibilities and kick me out.

For reference, I started the day in good spirits at the prospect of
having several unencumbered hours to work on framesync. Instead, I have
been so upset and disgusted by how this turned out that I was unable to
produce a single line of valuable code, be it for FFmpeg or my own
projects. And if you think that this is a paltry issue to be upset,
remember that the straw that breaks the camel's back does not need to be
heavy.

That is all I have to say on the matter. If necessary, details would
probably better be asked in private.

Regards,
Marton Balint Jan. 30, 2017, 12:09 a.m.
On Sat, 28 Jan 2017, Nicolas George wrote:

> Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :
>> so the behavior will be similar to
>> av_frame_make_writable().
>
> The point was to move away from that. Who copies a struct when you can
> move a pointer?
>

By the way, why av_frame_make_writable copies the struct?

As far as I see it can be implemented just like this:

int av_frame_make_writable(AVFrame *frame)
{
     int ret;
     int i;

     if (!frame->buf[0])
         return AVERROR(EINVAL);

     for (i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++) {
         if (frame->buf[i]) {
             ret = av_buffer_make_writable(&frame->buf[i]);
             if (ret < 0)
                 return ret;
             frame->data[i] = frame->buf[i]->data;
         }
     }
     for (i = 0; i < frame->nb_extended_buf; i++) {
         ret = av_buffer_make_writable(&frame->extended_buf[i]);
         if (ret < 0)
             return ret;
         frame->extended_data[i] = frame->extended_buf[i]->data;
     }

     return 0;
}

It even passes fate. What do I miss?

Don't get me wrong, I know that this approach cannot be implemented 
directly into the filtering case, because of the custom get buffer 
callback and the frame pool, but for the generic frame function, is there 
any downside doing this?

Thanks,
Marton
Rostislav Pehlivanov Jan. 30, 2017, 12:56 a.m.
On 30 January 2017 at 00:09, Marton Balint <cus@passwd.hu> wrote:

>
> On Sat, 28 Jan 2017, Nicolas George wrote:
>
> Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :
>>
>>> so the behavior will be similar to
>>> av_frame_make_writable().
>>>
>>
>> The point was to move away from that. Who copies a struct when you can
>> move a pointer?
>>
>>
> By the way, why av_frame_make_writable copies the struct?
>
> As far as I see it can be implemented just like this:
>
> int av_frame_make_writable(AVFrame *frame)
> {
>     int ret;
>     int i;
>
>     if (!frame->buf[0])
>         return AVERROR(EINVAL);
>
>     for (i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++) {
>         if (frame->buf[i]) {
>             ret = av_buffer_make_writable(&frame->buf[i]);
>             if (ret < 0)
>                 return ret;
>             frame->data[i] = frame->buf[i]->data;
>         }
>     }
>     for (i = 0; i < frame->nb_extended_buf; i++) {
>         ret = av_buffer_make_writable(&frame->extended_buf[i]);
>         if (ret < 0)
>             return ret;
>         frame->extended_data[i] = frame->extended_buf[i]->data;
>     }
>
>     return 0;
> }
>
> It even passes fate. What do I miss?
>
> Don't get me wrong, I know that this approach cannot be implemented
> directly into the filtering case, because of the custom get buffer callback
> and the frame pool, but for the generic frame function, is there any
> downside doing this?
>
> Thanks,
> Marton
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

The memory referenced by the AVFrame can be e.g. in hardware or used by
something else. It makes no sense to try to write to it, you'd just mess up
whatever else the user is using the data for.
So you allocate new buffers, make a copy of the data and unref the ref you
have.
Hendrik Leppkes Jan. 30, 2017, 4:39 a.m.
On Sun, Jan 29, 2017 at 7:43 PM, Nicolas George <george@nsup.org> wrote:
>
> As for myself, I do not wish to have any further interactions at all
> with wm4. Starting now, as far as I am concerned, for all intents and
> purposes, they and their messages no longer exist. I will reconsider my
> position if I learn they can go for a year without badmouthing the
> project or mocking or disparaging any of its contributors.
>
> Of course, that means that all their proposals on areas of code for
> which I am responsible are silently rejected. If somebody else sponsors
> such a change, I will discuss it as if it were their own with all the
> good-will that I am capable of.
>
> If another member of the project considers this stance unacceptable, you
> can take your responsibilities and kick me out.

This stance is absolutely unacceptable. You don't get to reject
patches or ignore feedback on some petty personal feud, or cherry-pick
the feedback you want to honor and which to ignore or even dismiss.
This behavior would go both against the code of conduct and our
established patch review regulations.

- Hendrik
wm4 Jan. 30, 2017, 6:42 a.m.
On Mon, 30 Jan 2017 01:09:27 +0100 (CET)
Marton Balint <cus@passwd.hu> wrote:

> On Sat, 28 Jan 2017, Nicolas George wrote:
> 
> > Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :  
> >> so the behavior will be similar to
> >> av_frame_make_writable().  
> >
> > The point was to move away from that. Who copies a struct when you can
> > move a pointer?
> >  
> 
> By the way, why av_frame_make_writable copies the struct?
> 
> As far as I see it can be implemented just like this:
> 
> int av_frame_make_writable(AVFrame *frame)
> {
>      int ret;
>      int i;
> 
>      if (!frame->buf[0])
>          return AVERROR(EINVAL);
> 
>      for (i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++) {
>          if (frame->buf[i]) {
>              ret = av_buffer_make_writable(&frame->buf[i]);
>              if (ret < 0)
>                  return ret;
>              frame->data[i] = frame->buf[i]->data;
>          }
>      }
>      for (i = 0; i < frame->nb_extended_buf; i++) {
>          ret = av_buffer_make_writable(&frame->extended_buf[i]);
>          if (ret < 0)
>              return ret;
>          frame->extended_data[i] = frame->extended_buf[i]->data;
>      }
> 
>      return 0;
> }
> 
> It even passes fate. What do I miss?
> 
> Don't get me wrong, I know that this approach cannot be implemented 
> directly into the filtering case, because of the custom get buffer 
> callback and the frame pool, but for the generic frame function, is there 
> any downside doing this?

data[i] doesn't have to point to the start of buf[i]. Also multiple
planes can be in the same buffer. I guess the function is implemented
this way because that's the simplest.
wm4 Jan. 30, 2017, 7:36 a.m.
On Sun, 29 Jan 2017 19:43:04 +0100
Nicolas George <george@nsup.org> wrote:

> Le decadi 10 pluviôse, an CCXXV, Michael Niedermayer a écrit :
> > also we need a maintainer for the libavfilter core or a area covering
> > avfilter.c and that person should then make a decission.  
> 
> I have more or less acted as the de-facto maintainer of all that has to
> do with scheduling in the lavfi framework. I can take over the rest of
> the work (that would have required, for example, giving more attention
> to the frame pool patches) if people want. I intended to propose when my
> work of overhauling the design would have been finished.
> 
> I will not propose it officially myself right now, especially not as a
> patch for the MAINTAINERS file, as it would look like escalating the
> game of core-wars. But if somebody else pushes a corresponding patch I
> will not object at all.
> 
> > But id like to ask everyone to NOT escalate this further, iam
> > sure that nothing good would come out of that.  
> 
> I fully agree.
> 
> wm4 has threatened to push a patch that I have explicitly and
> unambiguously rejected. Furthermore, Muhammad, the original author of
> this patch, has approved an alternate solution for fixing the same
> issue.
> 
> Under these circumstances, pushing the patch would be a deliberate act
> of escalation.

Well, in the quoted paragraph below you write that you will probably
push the discussed patch yourself. So for that reason and for the sake
of deescalation, I won't push it. I'll probably bring the patch up in a
few weeks or so to avoid that the patch gets lost. (Because it's a good
change, even if somewhat minor and not really worth all this stupid
drama.)

> Note: I rejected the patch based on the main change it advertises, the
> change in the function interface. The patch also contains changes making
> the implementation simpler. These change are very worthy. If Muhammad
> proposes them separately I will gladly approve and apply; and if not I
> will eventually do it myself.

So you agree with the patch, except you didn't say that, and instead
publicly rejected it? There is no logic in this.

I can only assume that you want ff_inlink_make_frame_writable to retain
its current signature. I'd argue against that, but I'm not getting the
chance to.

> >							       as i do
> > not want to take a side  
> 
> When witnessing a bullying situation, not taking a side amounts to
> siding with the bully. I will leave the readers decide for themselves if
> and how that statement applies to the current situation.

You are the "bully" - I've witnessed it countless of times.

> As for myself, I do not wish to have any further interactions at all
> with wm4. Starting now, as far as I am concerned, for all intents and
> purposes, they and their messages no longer exist. I will reconsider my
> position if I learn they can go for a year without badmouthing the
> project or mocking or disparaging any of its contributors.

It doesn't work this way. This is an open source project. If you wish
for me not to exist, you can either murder me (please don't), or start
working on another project. If you want to stay, I promise I will try
to avoid being insulting (if you're nice to me as well).

If you push more patches while ignoring my reviews (which you already
did once), then there's a problem. As you've put it so nicely, pushing
patches under those circumstances would be deliberate acts of
escalation.

> Of course, that means that all their proposals on areas of code for
> which I am responsible are silently rejected. If somebody else sponsors
> such a change, I will discuss it as if it were their own with all the
> good-will that I am capable of.

I know you want to portray yourself as victim, but I'm sorry, with
such clear-cut words under thinly veiled hostility and a barely working
pretense of trying to appear reasonable, it just doesn't work.

With that stance, you becoming the maintainer of anything in FFmpeg is
clearly unacceptable. Hell, even your reviews could not be trusted at
all, since you just reject patches because you're playing some shitty
troll game, instead of staying technical.

"Silent rejection" doesn't exist. It doesn't work that way - you can't
make others to further your little war with me, just so you can pretend
I don't exist.

> If another member of the project considers this stance unacceptable, you
> can take your responsibilities and kick me out.

Well, one already did (see the other reply to this mail).

I'm also a project member and find this unacceptable, but I know you
won't count me for some reason.

> For reference, I started the day in good spirits at the prospect of
> having several unencumbered hours to work on framesync. Instead, I have
> been so upset and disgusted by how this turned out that I was unable to
> produce a single line of valuable code, be it for FFmpeg or my own
> projects. And if you think that this is a paltry issue to be upset,
> remember that the straw that breaks the camel's back does not need to be
> heavy.

My day is ruined as well. I can't help to notice how accusatory your
is against me. It's as if I did something really bad, that stops your
productivity (and, like the implication could be, robbed the FFmpeg
project of your valuable changes). But you also say "how this turned
out", which implies you're not happy with the other's reactions, and
how blame goes to them as well.

I would have tried to avoid engaging you, but you have aggressively
tried to prevent this patch from being pushed. What did I do? I argued
that Muhammad's patch was a good one. I gave technical reasons as to
why. You remained stubborn and used some of your passive-aggressive
thinly veiled insults. I admit I trolled back. But hey, at least I had
some actual arguments. Whenever it was your time to respond to actual
technical issues, you ignored them, and decided to claim that something
about my post was "rude" - despite doing that yourself (like saying
things like "wm4's bad advice").

I haven't rejected your alternative patch to fix this either. I only
argued that it'd be nice to push Muhammad Faiz's first patch (which,
by the way, you think is (at least partially) a "worthy" change).

But here you go. Just after someone asked for deescalation, you are
writing an email how you will ignore all of my future mails, and
explicitly stated above that you will reject future changes from me.

You feel sick? You're doing this to yourself.

> That is all I have to say on the matter. If necessary, details would
> probably better be asked in private.
> 
> Regards,
>
Marton Balint Jan. 30, 2017, 7:38 p.m.
On Mon, 30 Jan 2017, Rostislav Pehlivanov wrote:

> On 30 January 2017 at 00:09, Marton Balint <cus@passwd.hu> wrote:
>
>>
>> On Sat, 28 Jan 2017, Nicolas George wrote:
>>
>> Le nonidi 9 pluviôse, an CCXXV, Muhammad Faiz a écrit :
>>>
>>>> so the behavior will be similar to
>>>> av_frame_make_writable().
>>>>
>>>
>>> The point was to move away from that. Who copies a struct when you can
>>> move a pointer?
>>>
>>>
>> By the way, why av_frame_make_writable copies the struct?
>>
>> As far as I see it can be implemented just like this:
>>
>> int av_frame_make_writable(AVFrame *frame)
>> {
>>     int ret;
>>     int i;
>>
>>     if (!frame->buf[0])
>>         return AVERROR(EINVAL);
>>
>>     for (i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++) {
>>         if (frame->buf[i]) {
>>             ret = av_buffer_make_writable(&frame->buf[i]);
>>             if (ret < 0)
>>                 return ret;
>>             frame->data[i] = frame->buf[i]->data;
>>         }
>>     }
>>     for (i = 0; i < frame->nb_extended_buf; i++) {
>>         ret = av_buffer_make_writable(&frame->extended_buf[i]);
>>         if (ret < 0)
>>             return ret;
>>         frame->extended_data[i] = frame->extended_buf[i]->data;
>>     }
>>
>>     return 0;
>> }
>>
>> It even passes fate. What do I miss?
>>
>> Don't get me wrong, I know that this approach cannot be implemented
>> directly into the filtering case, because of the custom get buffer callback
>> and the frame pool, but for the generic frame function, is there any
>> downside doing this?
>>
>> Thanks,
>> Marton
>>
> The memory referenced by the AVFrame can be e.g. in hardware or used by
> something else. It makes no sense to try to write to it, you'd just mess up
> whatever else the user is using the data for.
> So you allocate new buffers, make a copy of the data and unref the ref you
> have.

I don't follow. The code above is also doing that, making writable all 
the AVBufferRef-s, therefore allocating a new buffer for them, copying 
their content to the newly allocated buffer, and unrefing the old buffer.

Regards,
Marton

Patch hide | download patch | download mbox

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index c12d491..c8dafd2 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -1104,7 +1104,7 @@  static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
         filter_frame = default_filter_frame;
 
     if (dst->needs_writable) {
-        ret = ff_inlink_make_frame_writable(link, &frame);
+        ret = ff_inlink_make_frame_writable(link, frame);
         if (ret < 0)
             goto fail;
     }
@@ -1556,9 +1556,8 @@  int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
     return 1;
 }
 
-int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe)
+int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame *frame)
 {
-    AVFrame *frame = *rframe;
     AVFrame *out;
     int ret;
 
@@ -1585,23 +1584,10 @@  int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe)
         return ret;
     }
 
-    switch (link->type) {
-    case AVMEDIA_TYPE_VIDEO:
-        av_image_copy(out->data, out->linesize, (const uint8_t **)frame->data, frame->linesize,
-                      frame->format, frame->width, frame->height);
-        break;
-    case AVMEDIA_TYPE_AUDIO:
-        av_samples_copy(out->extended_data, frame->extended_data,
-                        0, 0, frame->nb_samples,
-                        av_frame_get_channels(frame),
-                        frame->format);
-        break;
-    default:
-        av_assert0(!"reached");
-    }
-
-    av_frame_free(&frame);
-    *rframe = out;
+    av_frame_copy(out, frame);
+    av_frame_unref(frame);
+    av_frame_move_ref(frame, out);
+    av_frame_free(&out);
     return 0;
 }
 
diff --git a/libavfilter/filters.h b/libavfilter/filters.h
index 2c78d60..5d32403 100644
--- a/libavfilter/filters.h
+++ b/libavfilter/filters.h
@@ -101,7 +101,7 @@  int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
  * This is similar to av_frame_make_writable() except it uses the link's
  * buffer allocation callback, and therefore allows direct rendering.
  */
-int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe);
+int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame *frame);
 
 /**
  * Test and acknowledge the change of status on the link.