diff mbox series

[FFmpeg-devel] fftools/ffplay: do not write out of rdft visualization texture

Message ID 20210303222722.14368-1-cus@passwd.hu
State Accepted
Commit 573f05a7533cd9aed3ed895b4fa4ad8fcba4e56a
Headers show
Series [FFmpeg-devel] fftools/ffplay: do not write out of rdft visualization texture | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Marton Balint March 3, 2021, 10:27 p.m. UTC
If the window is resized it was possible that xpos pointed outside the
visualization texture. By rearranging the overflow check we make sure this (and
a crash) does not happen.

We also don't have to use xleft for start position, as that is 0 anyways, and
if we ever want to take into account xleft then the texture should be
positioned accordingly when rendering.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 fftools/ffplay.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer March 7, 2021, 10:28 p.m. UTC | #1
On Wed, Mar 03, 2021 at 11:27:22PM +0100, Marton Balint wrote:
> If the window is resized it was possible that xpos pointed outside the
> visualization texture. By rearranging the overflow check we make sure this (and
> a crash) does not happen.
> 

> We also don't have to use xleft for start position, as that is 0 anyways, and
> if we ever want to take into account xleft then the texture should be
> positioned accordingly when rendering.

reading this, i wonder if a assertion with xleft == 0 would make sense 

thx

[...]
Marton Balint March 8, 2021, 8:56 p.m. UTC | #2
On Sun, 7 Mar 2021, Michael Niedermayer wrote:

> On Wed, Mar 03, 2021 at 11:27:22PM +0100, Marton Balint wrote:
>> If the window is resized it was possible that xpos pointed outside the
>> visualization texture. By rearranging the overflow check we make sure this (and
>> a crash) does not happen.
>>
>
>> We also don't have to use xleft for start position, as that is 0 anyways, and
>> if we ever want to take into account xleft then the texture should be
>> positioned accordingly when rendering.
>
> reading this, i wonder if a assertion with xleft == 0 would make sense

I don't really see the point. I'd rather add the xleft/ytop to the render 
if you prefer, but overall I don't think it matters.

--- a/fftools/ffplay.c
+++ b/fftools/ffplay.c
@@ -1193,7 +1193,8 @@ static void video_audio_display(VideoState *s)
                  }
                  SDL_UnlockTexture(s->vis_texture);
              }
-            SDL_RenderCopy(renderer, s->vis_texture, NULL, NULL);
+            rect = (SDL_Rect){s->xleft, s->ytop, s->width, s->height};
+            SDL_RenderCopy(renderer, s->vis_texture, NULL, &rect);
          }
          if (!s->paused)
              s->xpos++;

Regards,
Marton
Michael Niedermayer March 8, 2021, 9:10 p.m. UTC | #3
On Mon, Mar 08, 2021 at 09:56:38PM +0100, Marton Balint wrote:
> 
> 
> On Sun, 7 Mar 2021, Michael Niedermayer wrote:
> 
> > On Wed, Mar 03, 2021 at 11:27:22PM +0100, Marton Balint wrote:
> > > If the window is resized it was possible that xpos pointed outside the
> > > visualization texture. By rearranging the overflow check we make sure this (and
> > > a crash) does not happen.
> > > 
> > 
> > > We also don't have to use xleft for start position, as that is 0 anyways, and
> > > if we ever want to take into account xleft then the texture should be
> > > positioned accordingly when rendering.
> > 
> > reading this, i wonder if a assertion with xleft == 0 would make sense
> 
> I don't really see the point. 

It was just an idea that came to my mind without any deep thoughts


> I'd rather add the xleft/ytop to the render if
> you prefer, but overall I don't think it matters.

please do what you prefer!

thx

[...]
Marton Balint March 10, 2021, 7:21 p.m. UTC | #4
On Mon, 8 Mar 2021, Michael Niedermayer wrote:

> On Mon, Mar 08, 2021 at 09:56:38PM +0100, Marton Balint wrote:
>>
>>
>> On Sun, 7 Mar 2021, Michael Niedermayer wrote:
>>
>>> On Wed, Mar 03, 2021 at 11:27:22PM +0100, Marton Balint wrote:
>>>> If the window is resized it was possible that xpos pointed outside the
>>>> visualization texture. By rearranging the overflow check we make sure this (and
>>>> a crash) does not happen.
>>>>
>>>
>>>> We also don't have to use xleft for start position, as that is 0 anyways, and
>>>> if we ever want to take into account xleft then the texture should be
>>>> positioned accordingly when rendering.
>>>
>>> reading this, i wonder if a assertion with xleft == 0 would make sense
>>
>> I don't really see the point.
>
> It was just an idea that came to my mind without any deep thoughts
>
>
>> I'd rather add the xleft/ytop to the render if
>> you prefer, but overall I don't think it matters.
>
> please do what you prefer!

Ok, thanks, pushed as is then.

Regards,
Marton
diff mbox series

Patch

diff --git a/fftools/ffplay.c b/fftools/ffplay.c
index b9a30cdb11..0cdadab293 100644
--- a/fftools/ffplay.c
+++ b/fftools/ffplay.c
@@ -1148,6 +1148,8 @@  static void video_audio_display(VideoState *s)
         if (realloc_texture(&s->vis_texture, SDL_PIXELFORMAT_ARGB8888, s->width, s->height, SDL_BLENDMODE_NONE, 1) < 0)
             return;
 
+        if (s->xpos >= s->width)
+            s->xpos = 0;
         nb_display_channels= FFMIN(nb_display_channels, 2);
         if (rdft_bits != s->rdft_bits) {
             av_rdft_end(s->rdft);
@@ -1197,8 +1199,6 @@  static void video_audio_display(VideoState *s)
         }
         if (!s->paused)
             s->xpos++;
-        if (s->xpos >= s->width)
-            s->xpos= s->xleft;
     }
 }