[FFmpeg-devel] avfilter/video: pick sar from link

Submitted by Paul B Mahol on Dec. 7, 2017, 9:23 a.m.

Details

Message ID 20171207092333.17016-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Dec. 7, 2017, 9:23 a.m.
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavfilter/video.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Nicolas George Dec. 7, 2017, 7:03 p.m.
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,
Paul B Mahol Dec. 7, 2017, 7:52 p.m.
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.
Paul B Mahol Dec. 8, 2017, 7:44 p.m.
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.
Paul B Mahol Dec. 9, 2017, 10:24 a.m.
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.
Nicolas George Dec. 9, 2017, 10:37 a.m.
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.
Paul B Mahol Dec. 9, 2017, 10:54 a.m.
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.
Nicolas George Dec. 10, 2017, 11:05 a.m.
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,
Paul B Mahol Dec. 10, 2017, 11:10 a.m.
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?
Nicolas George Dec. 10, 2017, 11:12 a.m.
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,
Paul B Mahol Dec. 10, 2017, 11:16 a.m.
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.
Nicolas George Dec. 10, 2017, 11:20 a.m.
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,
Paul B Mahol Dec. 10, 2017, 11:32 a.m.
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.

Patch hide | download patch | download mbox

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)