| Message ID | 20171207092333.17016-1-onemda@gmail.com |
|---|---|
| State | Accepted |
| Commit | 4754d70a23255bb378975258eff296d9bdebb687 |
| Headers | show |
Paul B Mahol (2017-12-07): > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavfilter/video.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) Please explain (in the commit message) why you think this patch is needed. Regards,
On 12/7/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-12-07): >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavfilter/video.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) > > Please explain (in the commit message) why you think this patch is > needed. It should not be needed for each filter that sets sample aspect ratio to set it explicitly also for each and every frame, instead that is automatically done in get_buffer call.
On 12/7/17, Paul B Mahol <onemda@gmail.com> wrote: > On 12/7/17, Nicolas George <george@nsup.org> wrote: >> Paul B Mahol (2017-12-07): >>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>> --- >>> libavfilter/video.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> Please explain (in the commit message) why you think this patch is >> needed. > > It should not be needed for each filter that sets sample aspect ratio > to set it explicitly also for each and every frame, instead that is > automatically done in get_buffer call. > Is this message ok? If yes then I will push this patch shortly.
On 12/8/17, Paul B Mahol <onemda@gmail.com> wrote: > On 12/7/17, Paul B Mahol <onemda@gmail.com> wrote: >> On 12/7/17, Nicolas George <george@nsup.org> wrote: >>> Paul B Mahol (2017-12-07): >>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>> --- >>>> libavfilter/video.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> Please explain (in the commit message) why you think this patch is >>> needed. >> >> It should not be needed for each filter that sets sample aspect ratio >> to set it explicitly also for each and every frame, instead that is >> automatically done in get_buffer call. >> > > Is this message ok? If yes then I will push this patch shortly. > Will push very soon.
Paul B Mahol (2017-12-09): > >> It should not be needed for each filter that sets sample aspect ratio > >> to set it explicitly also for each and every frame, instead that is > >> automatically done in get_buffer call. > > Is this message ok? If yes then I will push this patch shortly. Ok, assuming you put it in the commit message. I think it would be better to accompany it with patches that actually require the change, though. > Will push very soon. PLEASE STOP DOING THAT. Waiting less than 48 hours is unacceptably short for that kind of change when there is a discussion. I had to stop a more urgent task to answer right now, and that annoys me a lot. YOU ARE NOT ALONE IN THIS PROJECT, AND PEOPLE DO NOT HAVE THE SAME SCHEDULE AS YOU.
On 12/9/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-12-09): >> >> It should not be needed for each filter that sets sample aspect ratio >> >> to set it explicitly also for each and every frame, instead that is >> >> automatically done in get_buffer call. >> > Is this message ok? If yes then I will push this patch shortly. > > Ok, assuming you put it in the commit message. I think it would be > better to accompany it with patches that actually require the change, > though. No new patches require it, it is for case such stereo3d and setsar and bunch of others that may need to explicitly set sample aspect ratio to each frame. > >> Will push very soon. > > PLEASE STOP DOING THAT. > > Waiting less than 48 hours is unacceptably short for that kind of change > when there is a discussion. I had to stop a more urgent task to answer > right now, and that annoys me a lot. Do you work in White House or in nuclear power plant? > > YOU ARE NOT ALONE IN THIS PROJECT, AND PEOPLE DO NOT HAVE THE SAME > SCHEDULE AS YOU.
Paul B Mahol (2017-12-09): > No new patches require it, it is for case such stereo3d and setsar and bunch > of others that may need to explicitly set sample aspect ratio to each frame. "May need" is not always a good way of designing changes. It would be much better if you actually changed these filters at the same time to use it. That way you can be more sure that you did not mess things up in a subtle way. > Do you work in White House or in nuclear power plant? You do not know anything about the use of my time, as I know nothing about yours. Therefore, kindly refrain from making any assumptions about me, as I refrain from making assumptions about you. You frequently commit changes too soon. A newcomer asked about reasonable delay not so long ago, someone answered quoting one week before insisting, except if the patch has some kind of urgency. It seems reasonable to me. And it applies to people who have commit rights too. One week. Regards,
On 12/10/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-12-09): >> No new patches require it, it is for case such stereo3d and setsar and >> bunch >> of others that may need to explicitly set sample aspect ratio to each >> frame. > > "May need" is not always a good way of designing changes. It would be > much better if you actually changed these filters at the same time to > use it. That way you can be more sure that you did not mess things up in > a subtle way. Is changing inlink parameters valid at all?
Paul B Mahol (2017-12-10): > > "May need" is not always a good way of designing changes. It would be > > much better if you actually changed these filters at the same time to > > use it. That way you can be more sure that you did not mess things up in > > a subtle way. > Is changing inlink parameters valid at all? I do not understand the question. Regards,
On 12/10/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-12-10): >> > "May need" is not always a good way of designing changes. It would be >> > much better if you actually changed these filters at the same time to >> > use it. That way you can be more sure that you did not mess things up in >> > a subtle way. >> Is changing inlink parameters valid at all? > > I do not understand the question. See vf_aspect.c, it changes inlink parameters. This patch is required for example if one change sar of first input and that value is not propagated to next links.
Paul B Mahol (2017-12-10): > See vf_aspect.c, it changes inlink parameters. > > This patch is required for example if one change sar of first input > and that value is not propagated to next links. I had not noticed that. The code in vf_aspect.c looks invalid to me, I think the change should be done on outlink, and in the outpad's config_prop's callback. Regards,
On 12/10/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-12-10): >> See vf_aspect.c, it changes inlink parameters. >> >> This patch is required for example if one change sar of first input >> and that value is not propagated to next links. > > I had not noticed that. The code in vf_aspect.c looks invalid to me, I > think the change should be done on outlink, and in the outpad's > config_prop's callback. Yes its looks fishy at best, will change and test it.
diff --git a/libavfilter/video.c b/libavfilter/video.c index 6f9020b9fe..7a8e587798 100644 --- a/libavfilter/video.c +++ b/libavfilter/video.c @@ -43,6 +43,7 @@ AVFrame *ff_null_get_video_buffer(AVFilterLink *link, int w, int h) AVFrame *ff_default_get_video_buffer(AVFilterLink *link, int w, int h) { + AVFrame *frame = NULL; int pool_width = 0; int pool_height = 0; int pool_align = 0; @@ -86,7 +87,13 @@ AVFrame *ff_default_get_video_buffer(AVFilterLink *link, int w, int h) } } - return ff_frame_pool_get(link->frame_pool); + frame = ff_frame_pool_get(link->frame_pool); + if (!frame) + return NULL; + + frame->sample_aspect_ratio = link->sample_aspect_ratio; + + return frame; } AVFrame *ff_get_video_buffer(AVFilterLink *link, int w, int h)
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavfilter/video.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)