avconv: Always initialize the opkt struct on streamcopy

Message ID 20170529115940.459-1-lu_zero@gentoo.org
State New
Headers show

Commit Message

Luca Barbato May 29, 2017, 11:59 a.m.
---
 avtools/avconv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

wm4 May 29, 2017, 12:43 p.m. | #1
On Mon, 29 May 2017 13:59:40 +0200
Luca Barbato <lu_zero@gentoo.org> wrote:

> ---
>  avtools/avconv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/avtools/avconv.c b/avtools/avconv.c
> index 719d289ff9..c30e1953ed 100644
> --- a/avtools/avconv.c
> +++ b/avtools/avconv.c
> @@ -1127,14 +1127,14 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>      int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->mux_timebase);
>      AVPacket opkt;
>  
> +    av_init_packet(&opkt);
> +
>      // EOF: flush output bitstream filters.
>      if (!pkt) {
>          output_packet(of, &opkt, ost, 1);
>          return;
>      }
>  
> -    av_init_packet(&opkt);
> -
>      if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
>          !ost->copy_initial_nonkeyframes)
>          return;

This doesn't initialize all AVPacket fields.
Luca Barbato May 29, 2017, 1:31 p.m. | #2
On 5/29/17 2:43 PM, wm4 wrote:
> On Mon, 29 May 2017 13:59:40 +0200
> Luca Barbato <lu_zero@gentoo.org> wrote:
> 
>> ---
>>  avtools/avconv.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/avtools/avconv.c b/avtools/avconv.c
>> index 719d289ff9..c30e1953ed 100644
>> --- a/avtools/avconv.c
>> +++ b/avtools/avconv.c
>> @@ -1127,14 +1127,14 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>>      int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->mux_timebase);
>>      AVPacket opkt;
>>  
>> +    av_init_packet(&opkt);
>> +
>>      // EOF: flush output bitstream filters.
>>      if (!pkt) {
>>          output_packet(of, &opkt, ost, 1);
>>          return;
>>      }
>>  
>> -    av_init_packet(&opkt);
>> -
>>      if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
>>          !ost->copy_initial_nonkeyframes)
>>          return;
> 
> This doesn't initialize all AVPacket fields.

I can throw in a { 0 } if you like it better :)

lu
Anton Khirnov May 30, 2017, 6:13 a.m. | #3
Quoting Luca Barbato (2017-05-29 15:31:34)
> On 5/29/17 2:43 PM, wm4 wrote:
> > On Mon, 29 May 2017 13:59:40 +0200
> > Luca Barbato <lu_zero@gentoo.org> wrote:
> > 
> >> ---
> >>  avtools/avconv.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/avtools/avconv.c b/avtools/avconv.c
> >> index 719d289ff9..c30e1953ed 100644
> >> --- a/avtools/avconv.c
> >> +++ b/avtools/avconv.c
> >> @@ -1127,14 +1127,14 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
> >>      int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->mux_timebase);
> >>      AVPacket opkt;
> >>  
> >> +    av_init_packet(&opkt);
> >> +
> >>      // EOF: flush output bitstream filters.
> >>      if (!pkt) {
> >>          output_packet(of, &opkt, ost, 1);
> >>          return;
> >>      }
> >>  
> >> -    av_init_packet(&opkt);
> >> -
> >>      if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
> >>          !ost->copy_initial_nonkeyframes)
> >>          return;
> > 
> > This doesn't initialize all AVPacket fields.
> 
> I can throw in a { 0 } if you like it better :)

Stab. It's not a question of "liking", there are correct things to do
and incorrect things to do. So do the correct thing.
Luca Barbato May 30, 2017, 8:48 a.m. | #4
On 5/30/17 8:13 AM, Anton Khirnov wrote:
> Quoting Luca Barbato (2017-05-29 15:31:34)
>> On 5/29/17 2:43 PM, wm4 wrote:
>>> On Mon, 29 May 2017 13:59:40 +0200
>>> Luca Barbato <lu_zero@gentoo.org> wrote:
>>>
>>>> ---
>>>>  avtools/avconv.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/avtools/avconv.c b/avtools/avconv.c
>>>> index 719d289ff9..c30e1953ed 100644
>>>> --- a/avtools/avconv.c
>>>> +++ b/avtools/avconv.c
>>>> @@ -1127,14 +1127,14 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>>>>      int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->mux_timebase);
>>>>      AVPacket opkt;
>>>>  
>>>> +    av_init_packet(&opkt);
>>>> +
>>>>      // EOF: flush output bitstream filters.
>>>>      if (!pkt) {
>>>>          output_packet(of, &opkt, ost, 1);
>>>>          return;
>>>>      }
>>>>  
>>>> -    av_init_packet(&opkt);
>>>> -
>>>>      if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
>>>>          !ost->copy_initial_nonkeyframes)
>>>>          return;
>>>
>>> This doesn't initialize all AVPacket fields.
>>
>> I can throw in a { 0 } if you like it better :)
> 
> Stab. It's not a question of "liking", there are correct things to do
> and incorrect things to do. So do the correct thing.
> 

I'll push with the AVPacket fully initialized.
wm4 May 30, 2017, 8:57 a.m. | #5
On Tue, 30 May 2017 08:13:38 +0200
Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Luca Barbato (2017-05-29 15:31:34)
> > On 5/29/17 2:43 PM, wm4 wrote:  
> > > On Mon, 29 May 2017 13:59:40 +0200
> > > Luca Barbato <lu_zero@gentoo.org> wrote:
> > >   
> > >> ---
> > >>  avtools/avconv.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/avtools/avconv.c b/avtools/avconv.c
> > >> index 719d289ff9..c30e1953ed 100644
> > >> --- a/avtools/avconv.c
> > >> +++ b/avtools/avconv.c
> > >> @@ -1127,14 +1127,14 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
> > >>      int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->mux_timebase);
> > >>      AVPacket opkt;
> > >>  
> > >> +    av_init_packet(&opkt);
> > >> +
> > >>      // EOF: flush output bitstream filters.
> > >>      if (!pkt) {
> > >>          output_packet(of, &opkt, ost, 1);
> > >>          return;
> > >>      }
> > >>  
> > >> -    av_init_packet(&opkt);
> > >> -
> > >>      if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
> > >>          !ost->copy_initial_nonkeyframes)
> > >>          return;  
> > > 
> > > This doesn't initialize all AVPacket fields.  
> > 
> > I can throw in a { 0 } if you like it better :)  
> 
> Stab. It's not a question of "liking", there are correct things to do
> and incorrect things to do. So do the correct thing.
> 

Indeed, it's a necessity, or you'll get just as much UB as before.
It's not just a style or other minor issue. Maybe I should have
mentioned it.
Vittorio Giovara May 30, 2017, 3:03 p.m. | #6
On Tue, May 30, 2017 at 4:48 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
> On 5/30/17 8:13 AM, Anton Khirnov wrote:
>> Quoting Luca Barbato (2017-05-29 15:31:34)
>>> On 5/29/17 2:43 PM, wm4 wrote:
>>>> On Mon, 29 May 2017 13:59:40 +0200
>>>> Luca Barbato <lu_zero@gentoo.org> wrote:
>>>>
>>>>> ---
>>>>>  avtools/avconv.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/avtools/avconv.c b/avtools/avconv.c
>>>>> index 719d289ff9..c30e1953ed 100644
>>>>> --- a/avtools/avconv.c
>>>>> +++ b/avtools/avconv.c
>>>>> @@ -1127,14 +1127,14 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>>>>>      int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->mux_timebase);
>>>>>      AVPacket opkt;
>>>>>
>>>>> +    av_init_packet(&opkt);
>>>>> +
>>>>>      // EOF: flush output bitstream filters.
>>>>>      if (!pkt) {
>>>>>          output_packet(of, &opkt, ost, 1);
>>>>>          return;
>>>>>      }
>>>>>
>>>>> -    av_init_packet(&opkt);
>>>>> -
>>>>>      if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
>>>>>          !ost->copy_initial_nonkeyframes)
>>>>>          return;
>>>>
>>>> This doesn't initialize all AVPacket fields.
>>>
>>> I can throw in a { 0 } if you like it better :)
>>
>> Stab. It's not a question of "liking", there are correct things to do
>> and incorrect things to do. So do the correct thing.
>>
>
> I'll push with the AVPacket fully initialized.

Maybe mention why this patch is needed in the commit log.

Patch

diff --git a/avtools/avconv.c b/avtools/avconv.c
index 719d289ff9..c30e1953ed 100644
--- a/avtools/avconv.c
+++ b/avtools/avconv.c
@@ -1127,14 +1127,14 @@  static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
     int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->mux_timebase);
     AVPacket opkt;
 
+    av_init_packet(&opkt);
+
     // EOF: flush output bitstream filters.
     if (!pkt) {
         output_packet(of, &opkt, ost, 1);
         return;
     }
 
-    av_init_packet(&opkt);
-
     if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
         !ost->copy_initial_nonkeyframes)
         return;