| Message ID | 1485617034-31431-1-git-send-email-mfcc64@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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,
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.
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
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,
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.)
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.
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.
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?
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.
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?
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.
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.
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.
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?
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.
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.
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.
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 [...]
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
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,
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
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.
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
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.
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, >
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
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.
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(-)