diff mbox series

[FFmpeg-devel,v2] fftools/ffmpeg: accelerate seeking while reading input at native frame rate

Message ID 20210704145023.3141-1-fulinjie@zju.edu.cn
State New
Headers show
Series [FFmpeg-devel,v2] fftools/ffmpeg: accelerate seeking while reading input at native frame rate
Related show

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

Linjie Fu July 4, 2021, 2:50 p.m. UTC
From: Linjie Fu <linjie.justin.fu@gmail.com>

Skip the logic of frame rate emulation until the input reaches the
specified start time.

Test CMD:
   $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -

Before the patch:
first time to got frame, it takes 257305 us
After this patch:
first time to got frame, it takes 48879 us

Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
---
[v2]: fixed the mixed declaration and code warning
Calculate the time to get the first frame:
https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
 fftools/ffmpeg.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Linjie Fu July 7, 2021, 1:42 a.m. UTC | #1
On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
>
> From: Linjie Fu <linjie.justin.fu@gmail.com>
>
> Skip the logic of frame rate emulation until the input reaches the
> specified start time.
>
> Test CMD:
>    $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
>
> Before the patch:
> first time to got frame, it takes 257305 us
> After this patch:
> first time to got frame, it takes 48879 us
>
> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> ---
> [v2]: fixed the mixed declaration and code warning
> Calculate the time to get the first frame:
> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
>  fftools/ffmpeg.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index e97d879cb3..c8849e4250 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
>  {
>      if (f->rate_emu) {
>          int i;
> +        int64_t pts;
> +        int64_t now;
>          for (i = 0; i < f->nb_streams; i++) {
>              InputStream *ist = input_streams[f->ist_index + i];
> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> -            int64_t now = av_gettime_relative() - ist->start;
> +            if (!ist->got_output)
> +                continue;
> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> +            now = av_gettime_relative() - ist->start;
>              if (pts > now)
>                  return AVERROR(EAGAIN);
>          }
> --
> 2.31.1
ping, thx.

- linjie
Linjie Fu July 18, 2021, 4:02 a.m. UTC | #2
On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
>
> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
> >
> > From: Linjie Fu <linjie.justin.fu@gmail.com>
> >
> > Skip the logic of frame rate emulation until the input reaches the
> > specified start time.
> >
> > Test CMD:
> >    $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
> >
> > Before the patch:
> > first time to got frame, it takes 257305 us
> > After this patch:
> > first time to got frame, it takes 48879 us
> >
> > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> > ---
> > [v2]: fixed the mixed declaration and code warning
> > Calculate the time to get the first frame:
> > https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
> >  fftools/ffmpeg.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index e97d879cb3..c8849e4250 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
> >  {
> >      if (f->rate_emu) {
> >          int i;
> > +        int64_t pts;
> > +        int64_t now;
> >          for (i = 0; i < f->nb_streams; i++) {
> >              InputStream *ist = input_streams[f->ist_index + i];
> > -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> > -            int64_t now = av_gettime_relative() - ist->start;
> > +            if (!ist->got_output)
> > +                continue;
> > +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> > +            now = av_gettime_relative() - ist->start;
> >              if (pts > now)
> >                  return AVERROR(EAGAIN);
> >          }
> > --
> > 2.31.1
> ping, thx.
>
Another ping, thx.
Gyan Doshi July 18, 2021, 4:23 a.m. UTC | #3
On 2021-07-18 09:32, Linjie Fu wrote:
> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
>>>
>>> Skip the logic of frame rate emulation until the input reaches the
>>> specified start time.
>>>
>>> Test CMD:
>>>     $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
>>>
>>> Before the patch:
>>> first time to got frame, it takes 257305 us
>>> After this patch:
>>> first time to got frame, it takes 48879 us
>>>
>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
>>> ---
>>> [v2]: fixed the mixed declaration and code warning
>>> Calculate the time to get the first frame:
>>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
>>>   fftools/ffmpeg.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>> index e97d879cb3..c8849e4250 100644
>>> --- a/fftools/ffmpeg.c
>>> +++ b/fftools/ffmpeg.c
>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
>>>   {
>>>       if (f->rate_emu) {
>>>           int i;
>>> +        int64_t pts;
>>> +        int64_t now;
>>>           for (i = 0; i < f->nb_streams; i++) {
>>>               InputStream *ist = input_streams[f->ist_index + i];
>>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>> -            int64_t now = av_gettime_relative() - ist->start;
>>> +            if (!ist->got_output)
>>> +                continue;
>>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>> +            now = av_gettime_relative() - ist->start;
>>>               if (pts > now)
>>>                   return AVERROR(EAGAIN);
>>>           }
>>> --
>>> 2.31.1
>> ping, thx.
>>
> Another ping, thx.

I pushed changes to this code yesterday. I don't think it's required 
anymore, but do test.

Regards,
Gyan
Linjie Fu July 18, 2021, 5:12 a.m. UTC | #4
Hi Gyan,
On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>
>
>
> On 2021-07-18 09:32, Linjie Fu wrote:
> > On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
> >> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
> >>> From: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>
> >>> Skip the logic of frame rate emulation until the input reaches the
> >>> specified start time.
> >>>
> >>> Test CMD:
> >>>     $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
> >>>
> >>> Before the patch:
> >>> first time to got frame, it takes 257305 us
> >>> After this patch:
> >>> first time to got frame, it takes 48879 us
> >>>
> >>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> >>> ---
> >>> [v2]: fixed the mixed declaration and code warning
> >>> Calculate the time to get the first frame:
> >>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
> >>>   fftools/ffmpeg.c | 8 ++++++--
> >>>   1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >>> index e97d879cb3..c8849e4250 100644
> >>> --- a/fftools/ffmpeg.c
> >>> +++ b/fftools/ffmpeg.c
> >>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
> >>>   {
> >>>       if (f->rate_emu) {
> >>>           int i;
> >>> +        int64_t pts;
> >>> +        int64_t now;
> >>>           for (i = 0; i < f->nb_streams; i++) {
> >>>               InputStream *ist = input_streams[f->ist_index + i];
> >>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> >>> -            int64_t now = av_gettime_relative() - ist->start;
> >>> +            if (!ist->got_output)
> >>> +                continue;
> >>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> >>> +            now = av_gettime_relative() - ist->start;
> >>>               if (pts > now)
> >>>                   return AVERROR(EAGAIN);
> >>>           }
> >>> --
> >>> 2.31.1
> >> ping, thx.
> >>
> > Another ping, thx.
>
> I pushed changes to this code yesterday. I don't think it's required
> anymore, but do test.
>

Thanks for the review, tested after applying the readrate patch, I'm
afraid that it's not identical as hope,
since ist->nb_packets would increase no matter input stream got output or not:

(lldb) p ist->nb_packets
(uint64_t) $4 = 1

(lldb) p ist->got_output
(int) $5 = 0

Hence we still need to add the check for  ist->got_output, or replace
the ist->nb_packets.
Also there is a new warning caught by the check in patchwork, probably
"mixed declaration and code warning".
Will send patches to rebase and fix.

- linjie
Gyan Doshi July 18, 2021, 5:21 a.m. UTC | #5
On 2021-07-18 10:42, Linjie Fu wrote:
> Hi Gyan,
> On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 2021-07-18 09:32, Linjie Fu wrote:
>>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
>>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
>>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>
>>>>> Skip the logic of frame rate emulation until the input reaches the
>>>>> specified start time.
>>>>>
>>>>> Test CMD:
>>>>>      $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
>>>>>
>>>>> Before the patch:
>>>>> first time to got frame, it takes 257305 us
>>>>> After this patch:
>>>>> first time to got frame, it takes 48879 us
>>>>>
>>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>> ---
>>>>> [v2]: fixed the mixed declaration and code warning
>>>>> Calculate the time to get the first frame:
>>>>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
>>>>>    fftools/ffmpeg.c | 8 ++++++--
>>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>>> index e97d879cb3..c8849e4250 100644
>>>>> --- a/fftools/ffmpeg.c
>>>>> +++ b/fftools/ffmpeg.c
>>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
>>>>>    {
>>>>>        if (f->rate_emu) {
>>>>>            int i;
>>>>> +        int64_t pts;
>>>>> +        int64_t now;
>>>>>            for (i = 0; i < f->nb_streams; i++) {
>>>>>                InputStream *ist = input_streams[f->ist_index + i];
>>>>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>>>> -            int64_t now = av_gettime_relative() - ist->start;
>>>>> +            if (!ist->got_output)
>>>>> +                continue;
>>>>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>>>> +            now = av_gettime_relative() - ist->start;
>>>>>                if (pts > now)
>>>>>                    return AVERROR(EAGAIN);
>>>>>            }
>>>>> --
>>>>> 2.31.1
>>>> ping, thx.
>>>>
>>> Another ping, thx.
>> I pushed changes to this code yesterday. I don't think it's required
>> anymore, but do test.
>>
> Thanks for the review, tested after applying the readrate patch, I'm
> afraid that it's not identical as hope,
> since ist->nb_packets would increase no matter input stream got output or not:
>
> (lldb) p ist->nb_packets
> (uint64_t) $4 = 1
>
> (lldb) p ist->got_output
> (int) $5 = 0
>
> Hence we still need to add the check for  ist->got_output, or replace
> the ist->nb_packets.

No, test the speed, not the parity of got_output. got_output is only 
incremented when the stream is decoded. Won't work with streamcopy.
nb_packets is the correct check since it's incremented after the initial 
packet is demuxed.

> Also there is a new warning caught by the check in patchwork, probably
> "mixed declaration and code warning".
> Will send patches to rebase and fix.

There is already patch for the mixed warning.

Regards,
Gyan
Linjie Fu July 18, 2021, 8:05 a.m. UTC | #6
On Sun, Jul 18, 2021 at 1:21 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>
>
>
> On 2021-07-18 10:42, Linjie Fu wrote:
> > Hi Gyan,
> > On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >>
> >>
> >> On 2021-07-18 09:32, Linjie Fu wrote:
> >>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
> >>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
> >>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>>>
> >>>>> Skip the logic of frame rate emulation until the input reaches the
> >>>>> specified start time.
> >>>>>
> >>>>> Test CMD:
> >>>>>      $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
> >>>>>
> >>>>> Before the patch:
> >>>>> first time to got frame, it takes 257305 us
> >>>>> After this patch:
> >>>>> first time to got frame, it takes 48879 us
> >>>>>
> >>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>>> ---
> >>>>> [v2]: fixed the mixed declaration and code warning
> >>>>> Calculate the time to get the first frame:
> >>>>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
> >>>>>    fftools/ffmpeg.c | 8 ++++++--
> >>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >>>>> index e97d879cb3..c8849e4250 100644
> >>>>> --- a/fftools/ffmpeg.c
> >>>>> +++ b/fftools/ffmpeg.c
> >>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
> >>>>>    {
> >>>>>        if (f->rate_emu) {
> >>>>>            int i;
> >>>>> +        int64_t pts;
> >>>>> +        int64_t now;
> >>>>>            for (i = 0; i < f->nb_streams; i++) {
> >>>>>                InputStream *ist = input_streams[f->ist_index + i];
> >>>>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> >>>>> -            int64_t now = av_gettime_relative() - ist->start;
> >>>>> +            if (!ist->got_output)
> >>>>> +                continue;
> >>>>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> >>>>> +            now = av_gettime_relative() - ist->start;
> >>>>>                if (pts > now)
> >>>>>                    return AVERROR(EAGAIN);
> >>>>>            }
> >>>>> --
> >>>>> 2.31.1
> >>>> ping, thx.
> >>>>
> >>> Another ping, thx.
> >> I pushed changes to this code yesterday. I don't think it's required
> >> anymore, but do test.
> >>
> > Thanks for the review, tested after applying the readrate patch, I'm
> > afraid that it's not identical as hope,
> > since ist->nb_packets would increase no matter input stream got output or not:
> >
> > (lldb) p ist->nb_packets
> > (uint64_t) $4 = 1
> >
> > (lldb) p ist->got_output
> > (int) $5 = 0
> >
> > Hence we still need to add the check for  ist->got_output, or replace
> > the ist->nb_packets.
>
> No, test the speed, not the parity of got_output. got_output is only
> incremented when the stream is decoded.

The time interval consumed to get the first frame is tested as well,
it's not speeded up:
First time to get the frame, it takes 257395 us.
After adding the check for got_output in decoding scenery, this would
reduce to 48792 us.

Hence it doesn't reduce the latency under decode scenery.

> Won't work with streamcopy.

Got your point.

> nb_packets is the correct check since it's incremented after the initial
> packet is demuxed.

IIRC, the initial packet is demuxed right after the first time we call
get_input_packet()-> av_read_frame(),  and nb_packets would increase.
Hence, the check for ist->nb_packets would reduce the latency for
demuxing the first packet,
and won't help much if we use start_time like "-ss 30". Under the
decoding scenery, we still
need to wait until the pkt->pts specified time then demux the packet.

If no objections, consider to change the check to:

if (!ist->nb_packets || (!ist->got_output && ist->decoding_needed)) continue;

to reduce the waiting time for:
1. demuxing the first packet (if any)
2. demuxing the packet before we got the first decoded output.

> > Also there is a new warning caught by the check in patchwork, probably
> > "mixed declaration and code warning".
> > Will send patches to rebase and fix.
>
> There is already patch for the mixed warning.
Saw it, thx.

- linjie
Gyan Doshi July 18, 2021, 9:25 a.m. UTC | #7
On 2021-07-18 13:35, Linjie Fu wrote:
> On Sun, Jul 18, 2021 at 1:21 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 2021-07-18 10:42, Linjie Fu wrote:
>>> Hi Gyan,
>>> On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>>>
>>>> On 2021-07-18 09:32, Linjie Fu wrote:
>>>>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
>>>>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
>>>>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>>>
>>>>>>> Skip the logic of frame rate emulation until the input reaches the
>>>>>>> specified start time.
>>>>>>>
>>>>>>> Test CMD:
>>>>>>>       $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
>>>>>>>
>>>>>>> Before the patch:
>>>>>>> first time to got frame, it takes 257305 us
>>>>>>> After this patch:
>>>>>>> first time to got frame, it takes 48879 us
>>>>>>>
>>>>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>>> ---
>>>>>>> [v2]: fixed the mixed declaration and code warning
>>>>>>> Calculate the time to get the first frame:
>>>>>>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
>>>>>>>     fftools/ffmpeg.c | 8 ++++++--
>>>>>>>     1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>>>>> index e97d879cb3..c8849e4250 100644
>>>>>>> --- a/fftools/ffmpeg.c
>>>>>>> +++ b/fftools/ffmpeg.c
>>>>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
>>>>>>>     {
>>>>>>>         if (f->rate_emu) {
>>>>>>>             int i;
>>>>>>> +        int64_t pts;
>>>>>>> +        int64_t now;
>>>>>>>             for (i = 0; i < f->nb_streams; i++) {
>>>>>>>                 InputStream *ist = input_streams[f->ist_index + i];
>>>>>>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>>>>>> -            int64_t now = av_gettime_relative() - ist->start;
>>>>>>> +            if (!ist->got_output)
>>>>>>> +                continue;
>>>>>>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>>>>>> +            now = av_gettime_relative() - ist->start;
>>>>>>>                 if (pts > now)
>>>>>>>                     return AVERROR(EAGAIN);
>>>>>>>             }
>>>>>>> --
>>>>>>> 2.31.1
>>>>>> ping, thx.
>>>>>>
>>>>> Another ping, thx.
>>>> I pushed changes to this code yesterday. I don't think it's required
>>>> anymore, but do test.
>>>>
>>> Thanks for the review, tested after applying the readrate patch, I'm
>>> afraid that it's not identical as hope,
>>> since ist->nb_packets would increase no matter input stream got output or not:
>>>
>>> (lldb) p ist->nb_packets
>>> (uint64_t) $4 = 1
>>>
>>> (lldb) p ist->got_output
>>> (int) $5 = 0
>>>
>>> Hence we still need to add the check for  ist->got_output, or replace
>>> the ist->nb_packets.
>> No, test the speed, not the parity of got_output. got_output is only
>> incremented when the stream is decoded.
> The time interval consumed to get the first frame is tested as well,
> it's not speeded up:
> First time to get the frame, it takes 257395 us.
> After adding the check for got_output in decoding scenery, this would
> reduce to 48792 us.
>
> Hence it doesn't reduce the latency under decode scenery.
>
>> Won't work with streamcopy.
> Got your point.
>
>> nb_packets is the correct check since it's incremented after the initial
>> packet is demuxed.
> IIRC, the initial packet is demuxed right after the first time we call
> get_input_packet()-> av_read_frame(),  and nb_packets would increase.
> Hence, the check for ist->nb_packets would reduce the latency for
> demuxing the first packet,
> and won't help much if we use start_time like "-ss 30". Under the
> decoding scenery, we still
> need to wait until the pkt->pts specified time then demux the packet.
>
> If no objections, consider to change the check to:
>
> if (!ist->nb_packets || (!ist->got_output && ist->decoding_needed)) continue;
>
> to reduce the waiting time for:
> 1. demuxing the first packet (if any)
> 2. demuxing the packet before we got the first decoded output.

Which sample file are you using, and how do I reproduce the timings?

Regards,
Gyan
Linjie Fu July 18, 2021, 10:07 a.m. UTC | #8
On Sun, Jul 18, 2021 at 5:26 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>
>
>
> On 2021-07-18 13:35, Linjie Fu wrote:
> > On Sun, Jul 18, 2021 at 1:21 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >>
> >>
> >> On 2021-07-18 10:42, Linjie Fu wrote:
> >>> Hi Gyan,
> >>> On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >>>>
> >>>> On 2021-07-18 09:32, Linjie Fu wrote:
> >>>>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
> >>>>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
> >>>>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>>>>>
> >>>>>>> Skip the logic of frame rate emulation until the input reaches the
> >>>>>>> specified start time.
> >>>>>>>
> >>>>>>> Test CMD:
> >>>>>>>       $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
> >>>>>>>
> >>>>>>> Before the patch:
> >>>>>>> first time to got frame, it takes 257305 us
> >>>>>>> After this patch:
> >>>>>>> first time to got frame, it takes 48879 us
> >>>>>>>
> >>>>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>>>>> ---
> >>>>>>> [v2]: fixed the mixed declaration and code warning
> >>>>>>> Calculate the time to get the first frame:
> >>>>>>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
> >>>>>>>     fftools/ffmpeg.c | 8 ++++++--
> >>>>>>>     1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >>>>>>> index e97d879cb3..c8849e4250 100644
> >>>>>>> --- a/fftools/ffmpeg.c
> >>>>>>> +++ b/fftools/ffmpeg.c
> >>>>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
> >>>>>>>     {
> >>>>>>>         if (f->rate_emu) {
> >>>>>>>             int i;
> >>>>>>> +        int64_t pts;
> >>>>>>> +        int64_t now;
> >>>>>>>             for (i = 0; i < f->nb_streams; i++) {
> >>>>>>>                 InputStream *ist = input_streams[f->ist_index + i];
> >>>>>>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> >>>>>>> -            int64_t now = av_gettime_relative() - ist->start;
> >>>>>>> +            if (!ist->got_output)
> >>>>>>> +                continue;
> >>>>>>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> >>>>>>> +            now = av_gettime_relative() - ist->start;
> >>>>>>>                 if (pts > now)
> >>>>>>>                     return AVERROR(EAGAIN);
> >>>>>>>             }
> >>>>>>> --
> >>>>>>> 2.31.1
> >>>>>> ping, thx.
> >>>>>>
> >>>>> Another ping, thx.
> >>>> I pushed changes to this code yesterday. I don't think it's required
> >>>> anymore, but do test.
> >>>>
> >>> Thanks for the review, tested after applying the readrate patch, I'm
> >>> afraid that it's not identical as hope,
> >>> since ist->nb_packets would increase no matter input stream got output or not:
> >>>
> >>> (lldb) p ist->nb_packets
> >>> (uint64_t) $4 = 1
> >>>
> >>> (lldb) p ist->got_output
> >>> (int) $5 = 0
> >>>
> >>> Hence we still need to add the check for  ist->got_output, or replace
> >>> the ist->nb_packets.
> >> No, test the speed, not the parity of got_output. got_output is only
> >> incremented when the stream is decoded.
> > The time interval consumed to get the first frame is tested as well,
> > it's not speeded up:
> > First time to get the frame, it takes 257395 us.
> > After adding the check for got_output in decoding scenery, this would
> > reduce to 48792 us.
> >
> > Hence it doesn't reduce the latency under decode scenery.
> >
> >> Won't work with streamcopy.
> > Got your point.
> >
> >> nb_packets is the correct check since it's incremented after the initial
> >> packet is demuxed.
> > IIRC, the initial packet is demuxed right after the first time we call
> > get_input_packet()-> av_read_frame(),  and nb_packets would increase.
> > Hence, the check for ist->nb_packets would reduce the latency for
> > demuxing the first packet,
> > and won't help much if we use start_time like "-ss 30". Under the
> > decoding scenery, we still
> > need to wait until the pkt->pts specified time then demux the packet.
> >
> > If no objections, consider to change the check to:
> >
> > if (!ist->nb_packets || (!ist->got_output && ist->decoding_needed)) continue;
> >
> > to reduce the waiting time for:
> > 1. demuxing the first packet (if any)
> > 2. demuxing the packet before we got the first decoded output.
>
> Which sample file are you using, and how do I reproduce the timings?
>
It could be reproduced with a random input file (if long enough), and
the time calculating method I used previously (needs to rebase a
little bit for now):
https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed

CMD:
$ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
or
$ffmpeg -re -ss 30 -i input.mp4 -f null -

- linjie
Gyan Doshi July 18, 2021, 12:57 p.m. UTC | #9
On 2021-07-18 15:37, Linjie Fu wrote:
> On Sun, Jul 18, 2021 at 5:26 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 2021-07-18 13:35, Linjie Fu wrote:
>>> On Sun, Jul 18, 2021 at 1:21 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>>>
>>>> On 2021-07-18 10:42, Linjie Fu wrote:
>>>>> Hi Gyan,
>>>>> On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>>>>> On 2021-07-18 09:32, Linjie Fu wrote:
>>>>>>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <linjie.justin.fu@gmail.com> wrote:
>>>>>>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn> wrote:
>>>>>>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>>>>>
>>>>>>>>> Skip the logic of frame rate emulation until the input reaches the
>>>>>>>>> specified start time.
>>>>>>>>>
>>>>>>>>> Test CMD:
>>>>>>>>>        $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
>>>>>>>>>
>>>>>>>>> Before the patch:
>>>>>>>>> first time to got frame, it takes 257305 us
>>>>>>>>> After this patch:
>>>>>>>>> first time to got frame, it takes 48879 us
>>>>>>>>>
>>>>>>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>>>>> ---
>>>>>>>>> [v2]: fixed the mixed declaration and code warning
>>>>>>>>> Calculate the time to get the first frame:
>>>>>>>>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
>>>>>>>>>      fftools/ffmpeg.c | 8 ++++++--
>>>>>>>>>      1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>>>>>>> index e97d879cb3..c8849e4250 100644
>>>>>>>>> --- a/fftools/ffmpeg.c
>>>>>>>>> +++ b/fftools/ffmpeg.c
>>>>>>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
>>>>>>>>>      {
>>>>>>>>>          if (f->rate_emu) {
>>>>>>>>>              int i;
>>>>>>>>> +        int64_t pts;
>>>>>>>>> +        int64_t now;
>>>>>>>>>              for (i = 0; i < f->nb_streams; i++) {
>>>>>>>>>                  InputStream *ist = input_streams[f->ist_index + i];
>>>>>>>>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>>>>>>>> -            int64_t now = av_gettime_relative() - ist->start;
>>>>>>>>> +            if (!ist->got_output)
>>>>>>>>> +                continue;
>>>>>>>>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>>>>>>>> +            now = av_gettime_relative() - ist->start;
>>>>>>>>>                  if (pts > now)
>>>>>>>>>                      return AVERROR(EAGAIN);
>>>>>>>>>              }
>>>>>>>>> --
>>>>>>>>> 2.31.1
>>>>>>>> ping, thx.
>>>>>>>>
>>>>>>> Another ping, thx.
>>>>>> I pushed changes to this code yesterday. I don't think it's required
>>>>>> anymore, but do test.
>>>>>>
>>>>> Thanks for the review, tested after applying the readrate patch, I'm
>>>>> afraid that it's not identical as hope,
>>>>> since ist->nb_packets would increase no matter input stream got output or not:
>>>>>
>>>>> (lldb) p ist->nb_packets
>>>>> (uint64_t) $4 = 1
>>>>>
>>>>> (lldb) p ist->got_output
>>>>> (int) $5 = 0
>>>>>
>>>>> Hence we still need to add the check for  ist->got_output, or replace
>>>>> the ist->nb_packets.
>>>> No, test the speed, not the parity of got_output. got_output is only
>>>> incremented when the stream is decoded.
>>> The time interval consumed to get the first frame is tested as well,
>>> it's not speeded up:
>>> First time to get the frame, it takes 257395 us.
>>> After adding the check for got_output in decoding scenery, this would
>>> reduce to 48792 us.
>>>
>>> Hence it doesn't reduce the latency under decode scenery.
>>>
>>>> Won't work with streamcopy.
>>> Got your point.
>>>
>>>> nb_packets is the correct check since it's incremented after the initial
>>>> packet is demuxed.
>>> IIRC, the initial packet is demuxed right after the first time we call
>>> get_input_packet()-> av_read_frame(),  and nb_packets would increase.
>>> Hence, the check for ist->nb_packets would reduce the latency for
>>> demuxing the first packet,
>>> and won't help much if we use start_time like "-ss 30". Under the
>>> decoding scenery, we still
>>> need to wait until the pkt->pts specified time then demux the packet.
>>>
>>> If no objections, consider to change the check to:
>>>
>>> if (!ist->nb_packets || (!ist->got_output && ist->decoding_needed)) continue;
>>>
>>> to reduce the waiting time for:
>>> 1. demuxing the first packet (if any)
>>> 2. demuxing the packet before we got the first decoded output.
>> Which sample file are you using, and how do I reproduce the timings?
>>
> It could be reproduced with a random input file (if long enough), and
> the time calculating method I used previously (needs to rebase a
> little bit for now):
> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
>
> CMD:
> $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
> or
> $ffmpeg -re -ss 30 -i input.mp4 -f null -

I don't see a difference. See attached text file.

Regards,
Gyan
Input file downloaded from https://download.blender.org/durian/trailer/sintel_trailer-720p.mp4

---PATCHED-1-----
static int get_input_packet(InputFile *f, AVPacket **pkt)
{
    if (f->readrate || f->rate_emu) {
        int i;
        int64_t file_start = copy_ts * (
                              (f->ctx->start_time != AV_NOPTS_VALUE ? f->ctx->start_time * !start_at_zero : 0) +
                              (f->start_time != AV_NOPTS_VALUE ? f->start_time : 0)
                             );
        float scale = f->rate_emu ? 1.0 : f->readrate;
        for (i = 0; i < f->nb_streams; i++) {
            InputStream *ist = input_streams[f->ist_index + i];
            int64_t stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
            int64_t now = (av_gettime_relative() - ist->start)*scale + stream_ts_offset;
av_log(NULL, AV_LOG_WARNING, "#%d:%d |nb_packets: %"PRId64" | pts: %"PRId64" |now: %"PRId64" |decoded: %d |got_output: %d\n", ist->file_index, i, ist->nb_packets, pts, now, ist->decoding_needed, ist->got_output);
            if (!ist->nb_packets) continue;
            if (pts > now)
                return AVERROR(EAGAIN);
        }
    }
--------

---PATCHED-2-----
static int get_input_packet(InputFile *f, AVPacket **pkt)
{
    if (f->readrate || f->rate_emu) {
        int i;
        int64_t file_start = copy_ts * (
                              (f->ctx->start_time != AV_NOPTS_VALUE ? f->ctx->start_time * !start_at_zero : 0) +
                              (f->start_time != AV_NOPTS_VALUE ? f->start_time : 0)
                             );
        float scale = f->rate_emu ? 1.0 : f->readrate;
        for (i = 0; i < f->nb_streams; i++) {
            InputStream *ist = input_streams[f->ist_index + i];
            int64_t stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
            int64_t now = (av_gettime_relative() - ist->start)*scale + stream_ts_offset;
av_log(NULL, AV_LOG_WARNING, "#%d:%d |nb_packets: %"PRId64" | pts: %"PRId64" |now: %"PRId64" |decoded: %d |got_output: %d\n", ist->file_index, i, ist->nb_packets, pts, now, ist->decoding_needed, ist->got_output);
            if (!ist->nb_packets || (ist->decoding_needed && !ist->got_output)) continue;
            if (pts > now)
                return AVERROR(EAGAIN);
        }
    }
--------
(Difference is between line 19 and line 42)


Decode command:
ffmpeg -re -ss 30 -i sintel_trailer-720p.mp4 -an -vframes 24 -f null - -v 24

Streamcopy command:
ffmpeg -re -ss 30 -i sintel_trailer-720p.mp4 -c copy -an -vframes 24 -f null - -v 24


Results
=======
(shown till first frame output)


PATCHED-1 Decoded

#0:0 |nb_packets: 0 | pts: 0 |now: 776 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 904 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 1 | pts: -2666667 |now: 1062 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 1178 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 2 | pts: -2625000 |now: 1779 |decoded: 1 |got_output: 0    <-- time to demux second packet = 1779 us
#0:1 |nb_packets: 0 | pts: 0 |now: 1898 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 3 | pts: -2583333 |now: 2460 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2597 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 4 | pts: -2541667 |now: 3184 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3318 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 5 | pts: -2500000 |now: 4045 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 4174 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 6 | pts: -2458333 |now: 4715 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 4873 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 7 | pts: -2416667 |now: 5386 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 5527 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 8 | pts: -2375000 |now: 6032 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 6164 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 9 | pts: -2333333 |now: 6701 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 6832 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 10 | pts: -2291667 |now: 7367 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 7722 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 11 | pts: -2250000 |now: 8411 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 8599 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 12 | pts: -2208333 |now: 9331 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 9507 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 13 | pts: -2166667 |now: 10186 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 10461 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 14 | pts: -2125000 |now: 10757 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 10894 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 15 | pts: -2041666 |now: 12543 |decoded: 1 |got_output: 1    <-- time to obtain first frame, after 15 pkts = 12543 us


PATCHED-2 Decoded

#0:0 |nb_packets: 0 | pts: 0 |now: 780 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 841 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 1 | pts: -2666667 |now: 932 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 992 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 2 | pts: -2625000 |now: 1645 |decoded: 1 |got_output: 0    <-- time to demux second packet = 1645 us
#0:1 |nb_packets: 0 | pts: 0 |now: 1747 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 3 | pts: -2583333 |now: 2311 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2371 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 4 | pts: -2541667 |now: 3058 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3123 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 5 | pts: -2500000 |now: 3864 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3929 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 6 | pts: -2458333 |now: 4558 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 4624 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 7 | pts: -2416667 |now: 5218 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 5281 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 8 | pts: -2375000 |now: 5886 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 6023 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 9 | pts: -2333333 |now: 6675 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 6801 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 10 | pts: -2291667 |now: 7774 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 7926 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 11 | pts: -2250000 |now: 8887 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 9026 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 12 | pts: -2208333 |now: 9624 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 9752 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 13 | pts: -2166667 |now: 10277 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 10423 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 14 | pts: -2125000 |now: 10847 |decoded: 1 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 10991 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 15 | pts: -2041666 |now: 12619 |decoded: 1 |got_output: 1    <-- time to obtain first frame, after 15 pkts = 12619 us


PATCHED-1 Streamcopied

#0:0 |nb_packets: 0 | pts: 0 |now: 77 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 198 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 1 | pts: -2666667 |now: 346 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 463 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 2 | pts: -2625000 |now: 585 |decoded: 0 |got_output: 0    <-- time to demux second packet = 585 us
#0:1 |nb_packets: 0 | pts: 0 |now: 701 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 3 | pts: -2583333 |now: 819 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 936 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 4 | pts: -2541667 |now: 1064 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 1181 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 5 | pts: -2500000 |now: 1295 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 1410 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 6 | pts: -2458333 |now: 1528 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 1644 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 7 | pts: -2416667 |now: 1760 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 1875 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 8 | pts: -2375000 |now: 1994 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2109 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 9 | pts: -2333333 |now: 2235 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2351 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 10 | pts: -2291667 |now: 2467 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2582 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 11 | pts: -2250000 |now: 2698 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2813 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 12 | pts: -2208333 |now: 2928 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3046 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 13 | pts: -2166667 |now: 3163 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3278 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 14 | pts: -2125000 |now: 3396 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3510 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 15 | pts: -2083333 |now: 3626 |decoded: 0 |got_output: 0    <-- time after 15 pkts = 3626 us


PATCHED-2 Streamcopied

#0:0 |nb_packets: 0 | pts: 0 |now: 74 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 191 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 1 | pts: -2666667 |now: 337 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 452 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 2 | pts: -2625000 |now: 575 |decoded: 0 |got_output: 0    <-- time to demux second packet = 575 us
#0:1 |nb_packets: 0 | pts: 0 |now: 691 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 3 | pts: -2583333 |now: 808 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 922 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 4 | pts: -2541667 |now: 1045 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 1160 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 5 | pts: -2500000 |now: 1309 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 1423 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 6 | pts: -2458333 |now: 1654 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 1865 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 7 | pts: -2416667 |now: 1987 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2103 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 8 | pts: -2375000 |now: 2219 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2334 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 9 | pts: -2333333 |now: 2458 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2574 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 10 | pts: -2291667 |now: 2691 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 2805 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 11 | pts: -2250000 |now: 2920 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3033 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 12 | pts: -2208333 |now: 3148 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3262 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 13 | pts: -2166667 |now: 3376 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3489 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 14 | pts: -2125000 |now: 3605 |decoded: 0 |got_output: 0
#0:1 |nb_packets: 0 | pts: 0 |now: 3718 |decoded: 0 |got_output: 0
#0:0 |nb_packets: 15 | pts: -2083333 |now: 3834 |decoded: 0 |got_output: 0    <-- time after 15 pkts = 3834 us
Linjie Fu July 18, 2021, 4:53 p.m. UTC | #10
On Sun, Jul 18, 2021 at 8:57 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>
>
>
> On 2021-07-18 15:37, Linjie Fu wrote:
> > On Sun, Jul 18, 2021 at 5:26 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >>
> >>
> >> On 2021-07-18 13:35, Linjie Fu wrote:
> >>> On Sun, Jul 18, 2021 at 1:21 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >>>>
> >>>> On 2021-07-18 10:42, Linjie Fu wrote:
> >>>>> Hi Gyan,
> >>>>> On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro>
wrote:
> >>>>>> On 2021-07-18 09:32, Linjie Fu wrote:
> >>>>>>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <
linjie.justin.fu@gmail.com> wrote:
> >>>>>>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn>
wrote:
> >>>>>>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>>>>>>>
> >>>>>>>>> Skip the logic of frame rate emulation until the input reaches
the
> >>>>>>>>> specified start time.
> >>>>>>>>>
> >>>>>>>>> Test CMD:
> >>>>>>>>>        $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2
-
> >>>>>>>>>
> >>>>>>>>> Before the patch:
> >>>>>>>>> first time to got frame, it takes 257305 us
> >>>>>>>>> After this patch:
> >>>>>>>>> first time to got frame, it takes 48879 us
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>>>>>>> ---
> >>>>>>>>> [v2]: fixed the mixed declaration and code warning
> >>>>>>>>> Calculate the time to get the first frame:
> >>>>>>>>>
https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
> >>>>>>>>>      fftools/ffmpeg.c | 8 ++++++--
> >>>>>>>>>      1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >>>>>>>>> index e97d879cb3..c8849e4250 100644
> >>>>>>>>> --- a/fftools/ffmpeg.c
> >>>>>>>>> +++ b/fftools/ffmpeg.c
> >>>>>>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile
*f, AVPacket **pkt)
> >>>>>>>>>      {
> >>>>>>>>>          if (f->rate_emu) {
> >>>>>>>>>              int i;
> >>>>>>>>> +        int64_t pts;
> >>>>>>>>> +        int64_t now;
> >>>>>>>>>              for (i = 0; i < f->nb_streams; i++) {
> >>>>>>>>>                  InputStream *ist = input_streams[f->ist_index
+ i];
> >>>>>>>>> -            int64_t pts = av_rescale(ist->dts, 1000000,
AV_TIME_BASE);
> >>>>>>>>> -            int64_t now = av_gettime_relative() - ist->start;
> >>>>>>>>> +            if (!ist->got_output)
> >>>>>>>>> +                continue;
> >>>>>>>>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> >>>>>>>>> +            now = av_gettime_relative() - ist->start;
> >>>>>>>>>                  if (pts > now)
> >>>>>>>>>                      return AVERROR(EAGAIN);
> >>>>>>>>>              }
> >>>>>>>>> --
> >>>>>>>>> 2.31.1
> >>>>>>>> ping, thx.
> >>>>>>>>
> >>>>>>> Another ping, thx.
> >>>>>> I pushed changes to this code yesterday. I don't think it's
required
> >>>>>> anymore, but do test.
> >>>>>>
> >>>>> Thanks for the review, tested after applying the readrate patch, I'm
> >>>>> afraid that it's not identical as hope,
> >>>>> since ist->nb_packets would increase no matter input stream got
output or not:
> >>>>>
> >>>>> (lldb) p ist->nb_packets
> >>>>> (uint64_t) $4 = 1
> >>>>>
> >>>>> (lldb) p ist->got_output
> >>>>> (int) $5 = 0
> >>>>>
> >>>>> Hence we still need to add the check for  ist->got_output, or
replace
> >>>>> the ist->nb_packets.
> >>>> No, test the speed, not the parity of got_output. got_output is only
> >>>> incremented when the stream is decoded.
> >>> The time interval consumed to get the first frame is tested as well,
> >>> it's not speeded up:
> >>> First time to get the frame, it takes 257395 us.
> >>> After adding the check for got_output in decoding scenery, this would
> >>> reduce to 48792 us.
> >>>
> >>> Hence it doesn't reduce the latency under decode scenery.
> >>>
> >>>> Won't work with streamcopy.
> >>> Got your point.
> >>>
> >>>> nb_packets is the correct check since it's incremented after the
initial
> >>>> packet is demuxed.
> >>> IIRC, the initial packet is demuxed right after the first time we call
> >>> get_input_packet()-> av_read_frame(),  and nb_packets would increase.
> >>> Hence, the check for ist->nb_packets would reduce the latency for
> >>> demuxing the first packet,
> >>> and won't help much if we use start_time like "-ss 30". Under the
> >>> decoding scenery, we still
> >>> need to wait until the pkt->pts specified time then demux the packet.
> >>>
> >>> If no objections, consider to change the check to:
> >>>
> >>> if (!ist->nb_packets || (!ist->got_output && ist->decoding_needed))
continue;
> >>>
> >>> to reduce the waiting time for:
> >>> 1. demuxing the first packet (if any)
> >>> 2. demuxing the packet before we got the first decoded output.
> >> Which sample file are you using, and how do I reproduce the timings?
> >>
> > It could be reproduced with a random input file (if long enough), and
> > the time calculating method I used previously (needs to rebase a
> > little bit for now):
> >
https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
> >
> > CMD:
> > $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
> > or
> > $ffmpeg -re -ss 30 -i input.mp4 -f null -
>
> I don't see a difference. See attached text file.
>

Checked the attached logs with decoding : ffmpeg -re -ss 30 -i
sintel_trailer-720p.mp4 -an -vframes 24 -f null - -v 24

#0:0 |nb_packets: 14 | pts: -2125000 |now: 10757 |decoded: 1 |got_output: 0
#0:0 |nb_packets: 15 | pts: -2041666 |now: 12543 |decoded: 1 |got_output: 1
   <-- time to obtain first frame, after 15 pkts = 12543 us

In this case the "pts" is always less than "now", hence there is no wait
and won't enter into the logic of  "EAGAIN return".
So no difference is observed.

Here is another example in which "pts" may be larger than "now" before we
got the output:
https://docs.google.com/document/d/19fTcGMvTC6surQTvJPNNnngUrTEIRnqElu2lCCU6qwU/edit?usp=sharing

- linjie
Gyan Doshi July 19, 2021, 7:18 a.m. UTC | #11
On 2021-07-18 22:23, Linjie Fu wrote:
> On Sun, Jul 18, 2021 at 8:57 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 2021-07-18 15:37, Linjie Fu wrote:
>>> On Sun, Jul 18, 2021 at 5:26 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>>>
>>>> On 2021-07-18 13:35, Linjie Fu wrote:
>>>>> On Sun, Jul 18, 2021 at 1:21 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>>>>> On 2021-07-18 10:42, Linjie Fu wrote:
>>>>>>> Hi Gyan,
>>>>>>> On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro>
> wrote:
>>>>>>>> On 2021-07-18 09:32, Linjie Fu wrote:
>>>>>>>>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <
> linjie.justin.fu@gmail.com> wrote:
>>>>>>>>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn>
> wrote:
>>>>>>>>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>>>>>>>
>>>>>>>>>>> Skip the logic of frame rate emulation until the input reaches
> the
>>>>>>>>>>> specified start time.
>>>>>>>>>>>
>>>>>>>>>>> Test CMD:
>>>>>>>>>>>         $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2
> -
>>>>>>>>>>> Before the patch:
>>>>>>>>>>> first time to got frame, it takes 257305 us
>>>>>>>>>>> After this patch:
>>>>>>>>>>> first time to got frame, it takes 48879 us
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>>>>>>> ---
>>>>>>>>>>> [v2]: fixed the mixed declaration and code warning
>>>>>>>>>>> Calculate the time to get the first frame:
>>>>>>>>>>>
> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
>>>>>>>>>>>       fftools/ffmpeg.c | 8 ++++++--
>>>>>>>>>>>       1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>>>>>>>>> index e97d879cb3..c8849e4250 100644
>>>>>>>>>>> --- a/fftools/ffmpeg.c
>>>>>>>>>>> +++ b/fftools/ffmpeg.c
>>>>>>>>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile
> *f, AVPacket **pkt)
>>>>>>>>>>>       {
>>>>>>>>>>>           if (f->rate_emu) {
>>>>>>>>>>>               int i;
>>>>>>>>>>> +        int64_t pts;
>>>>>>>>>>> +        int64_t now;
>>>>>>>>>>>               for (i = 0; i < f->nb_streams; i++) {
>>>>>>>>>>>                   InputStream *ist = input_streams[f->ist_index
> + i];
>>>>>>>>>>> -            int64_t pts = av_rescale(ist->dts, 1000000,
> AV_TIME_BASE);
>>>>>>>>>>> -            int64_t now = av_gettime_relative() - ist->start;
>>>>>>>>>>> +            if (!ist->got_output)
>>>>>>>>>>> +                continue;
>>>>>>>>>>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>>>>>>>>>>> +            now = av_gettime_relative() - ist->start;
>>>>>>>>>>>                   if (pts > now)
>>>>>>>>>>>                       return AVERROR(EAGAIN);
>>>>>>>>>>>               }
>>>>>>>>>>> --
>>>>>>>>>>> 2.31.1
>>>>>>>>>> ping, thx.
>>>>>>>>>>
>>>>>>>>> Another ping, thx.
>>>>>>>> I pushed changes to this code yesterday. I don't think it's
> required
>>>>>>>> anymore, but do test.
>>>>>>>>
>>>>>>> Thanks for the review, tested after applying the readrate patch, I'm
>>>>>>> afraid that it's not identical as hope,
>>>>>>> since ist->nb_packets would increase no matter input stream got
> output or not:
>>>>>>> (lldb) p ist->nb_packets
>>>>>>> (uint64_t) $4 = 1
>>>>>>>
>>>>>>> (lldb) p ist->got_output
>>>>>>> (int) $5 = 0
>>>>>>>
>>>>>>> Hence we still need to add the check for  ist->got_output, or
> replace
>>>>>>> the ist->nb_packets.
>>>>>> No, test the speed, not the parity of got_output. got_output is only
>>>>>> incremented when the stream is decoded.
>>>>> The time interval consumed to get the first frame is tested as well,
>>>>> it's not speeded up:
>>>>> First time to get the frame, it takes 257395 us.
>>>>> After adding the check for got_output in decoding scenery, this would
>>>>> reduce to 48792 us.
>>>>>
>>>>> Hence it doesn't reduce the latency under decode scenery.
>>>>>
>>>>>> Won't work with streamcopy.
>>>>> Got your point.
>>>>>
>>>>>> nb_packets is the correct check since it's incremented after the
> initial
>>>>>> packet is demuxed.
>>>>> IIRC, the initial packet is demuxed right after the first time we call
>>>>> get_input_packet()-> av_read_frame(),  and nb_packets would increase.
>>>>> Hence, the check for ist->nb_packets would reduce the latency for
>>>>> demuxing the first packet,
>>>>> and won't help much if we use start_time like "-ss 30". Under the
>>>>> decoding scenery, we still
>>>>> need to wait until the pkt->pts specified time then demux the packet.
>>>>>
>>>>> If no objections, consider to change the check to:
>>>>>
>>>>> if (!ist->nb_packets || (!ist->got_output && ist->decoding_needed))
> continue;
>>>>> to reduce the waiting time for:
>>>>> 1. demuxing the first packet (if any)
>>>>> 2. demuxing the packet before we got the first decoded output.
>>>> Which sample file are you using, and how do I reproduce the timings?
>>>>
>>> It could be reproduced with a random input file (if long enough), and
>>> the time calculating method I used previously (needs to rebase a
>>> little bit for now):
>>>
> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
>>> CMD:
>>> $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
>>> or
>>> $ffmpeg -re -ss 30 -i input.mp4 -f null -
>> I don't see a difference. See attached text file.
>>
> Checked the attached logs with decoding : ffmpeg -re -ss 30 -i
> sintel_trailer-720p.mp4 -an -vframes 24 -f null - -v 24
>
> #0:0 |nb_packets: 14 | pts: -2125000 |now: 10757 |decoded: 1 |got_output: 0
> #0:0 |nb_packets: 15 | pts: -2041666 |now: 12543 |decoded: 1 |got_output: 1
>     <-- time to obtain first frame, after 15 pkts = 12543 us
>
> In this case the "pts" is always less than "now", hence there is no wait
> and won't enter into the logic of  "EAGAIN return".
> So no difference is observed.
>
> Here is another example in which "pts" may be larger than "now" before we
> got the output:
> https://docs.google.com/document/d/19fTcGMvTC6surQTvJPNNnngUrTEIRnqElu2lCCU6qwU/edit?usp=sharing

Yeah, huge difference. Will adjust commit msg and push, since seeking is 
not what's affected here.

Regards,
Gyan
Gyan Doshi July 19, 2021, 7:31 a.m. UTC | #12
On 2021-07-19 12:48, Gyan Doshi wrote:
>
>
> On 2021-07-18 22:23, Linjie Fu wrote:
>> On Sun, Jul 18, 2021 at 8:57 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>>
>>>
>>> On 2021-07-18 15:37, Linjie Fu wrote:
>>>> On Sun, Jul 18, 2021 at 5:26 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>>>>
>>>>> On 2021-07-18 13:35, Linjie Fu wrote:
>>>>>> On Sun, Jul 18, 2021 at 1:21 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>>>>>> On 2021-07-18 10:42, Linjie Fu wrote:
>>>>>>>> Hi Gyan,
>>>>>>>> On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro>
>> wrote:
>>>>>>>>> On 2021-07-18 09:32, Linjie Fu wrote:
>>>>>>>>>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <
>> linjie.justin.fu@gmail.com> wrote:
>>>>>>>>>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn>
>> wrote:
>>>>>>>>>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Skip the logic of frame rate emulation until the input reaches
>> the
>>>>>>>>>>>> specified start time.
>>>>>>>>>>>>
>>>>>>>>>>>> Test CMD:
>>>>>>>>>>>>         $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f 
>>>>>>>>>>>> sdl2
>> -
>>>>>>>>>>>> Before the patch:
>>>>>>>>>>>> first time to got frame, it takes 257305 us
>>>>>>>>>>>> After this patch:
>>>>>>>>>>>> first time to got frame, it takes 48879 us
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> [v2]: fixed the mixed declaration and code warning
>>>>>>>>>>>> Calculate the time to get the first frame:
>>>>>>>>>>>>
>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed 
>>
>>>>>>>>>>>>       fftools/ffmpeg.c | 8 ++++++--
>>>>>>>>>>>>       1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>>>>>>>>>> index e97d879cb3..c8849e4250 100644
>>>>>>>>>>>> --- a/fftools/ffmpeg.c
>>>>>>>>>>>> +++ b/fftools/ffmpeg.c
>>>>>>>>>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile
>> *f, AVPacket **pkt)
>>>>>>>>>>>>       {
>>>>>>>>>>>>           if (f->rate_emu) {
>>>>>>>>>>>>               int i;
>>>>>>>>>>>> +        int64_t pts;
>>>>>>>>>>>> +        int64_t now;
>>>>>>>>>>>>               for (i = 0; i < f->nb_streams; i++) {
>>>>>>>>>>>>                   InputStream *ist = 
>>>>>>>>>>>> input_streams[f->ist_index
>> + i];
>>>>>>>>>>>> -            int64_t pts = av_rescale(ist->dts, 1000000,
>> AV_TIME_BASE);
>>>>>>>>>>>> -            int64_t now = av_gettime_relative() - ist->start;
>>>>>>>>>>>> +            if (!ist->got_output)
>>>>>>>>>>>> +                continue;
>>>>>>>>>>>> +            pts = av_rescale(ist->dts, 1000000, 
>>>>>>>>>>>> AV_TIME_BASE);
>>>>>>>>>>>> +            now = av_gettime_relative() - ist->start;
>>>>>>>>>>>>                   if (pts > now)
>>>>>>>>>>>>                       return AVERROR(EAGAIN);
>>>>>>>>>>>>               }
>>>>>>>>>>>> -- 
>>>>>>>>>>>> 2.31.1
>>>>>>>>>>> ping, thx.
>>>>>>>>>>>
>>>>>>>>>> Another ping, thx.
>>>>>>>>> I pushed changes to this code yesterday. I don't think it's
>> required
>>>>>>>>> anymore, but do test.
>>>>>>>>>
>>>>>>>> Thanks for the review, tested after applying the readrate 
>>>>>>>> patch, I'm
>>>>>>>> afraid that it's not identical as hope,
>>>>>>>> since ist->nb_packets would increase no matter input stream got
>> output or not:
>>>>>>>> (lldb) p ist->nb_packets
>>>>>>>> (uint64_t) $4 = 1
>>>>>>>>
>>>>>>>> (lldb) p ist->got_output
>>>>>>>> (int) $5 = 0
>>>>>>>>
>>>>>>>> Hence we still need to add the check for ist->got_output, or
>> replace
>>>>>>>> the ist->nb_packets.
>>>>>>> No, test the speed, not the parity of got_output. got_output is 
>>>>>>> only
>>>>>>> incremented when the stream is decoded.
>>>>>> The time interval consumed to get the first frame is tested as well,
>>>>>> it's not speeded up:
>>>>>> First time to get the frame, it takes 257395 us.
>>>>>> After adding the check for got_output in decoding scenery, this 
>>>>>> would
>>>>>> reduce to 48792 us.
>>>>>>
>>>>>> Hence it doesn't reduce the latency under decode scenery.
>>>>>>
>>>>>>> Won't work with streamcopy.
>>>>>> Got your point.
>>>>>>
>>>>>>> nb_packets is the correct check since it's incremented after the
>> initial
>>>>>>> packet is demuxed.
>>>>>> IIRC, the initial packet is demuxed right after the first time we 
>>>>>> call
>>>>>> get_input_packet()-> av_read_frame(),  and nb_packets would 
>>>>>> increase.
>>>>>> Hence, the check for ist->nb_packets would reduce the latency for
>>>>>> demuxing the first packet,
>>>>>> and won't help much if we use start_time like "-ss 30". Under the
>>>>>> decoding scenery, we still
>>>>>> need to wait until the pkt->pts specified time then demux the 
>>>>>> packet.
>>>>>>
>>>>>> If no objections, consider to change the check to:
>>>>>>
>>>>>> if (!ist->nb_packets || (!ist->got_output && ist->decoding_needed))
>> continue;
>>>>>> to reduce the waiting time for:
>>>>>> 1. demuxing the first packet (if any)
>>>>>> 2. demuxing the packet before we got the first decoded output.
>>>>> Which sample file are you using, and how do I reproduce the timings?
>>>>>
>>>> It could be reproduced with a random input file (if long enough), and
>>>> the time calculating method I used previously (needs to rebase a
>>>> little bit for now):
>>>>
>> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed 
>>
>>>> CMD:
>>>> $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
>>>> or
>>>> $ffmpeg -re -ss 30 -i input.mp4 -f null -
>>> I don't see a difference. See attached text file.
>>>
>> Checked the attached logs with decoding : ffmpeg -re -ss 30 -i
>> sintel_trailer-720p.mp4 -an -vframes 24 -f null - -v 24
>>
>> #0:0 |nb_packets: 14 | pts: -2125000 |now: 10757 |decoded: 1 
>> |got_output: 0
>> #0:0 |nb_packets: 15 | pts: -2041666 |now: 12543 |decoded: 1 
>> |got_output: 1
>>     <-- time to obtain first frame, after 15 pkts = 12543 us
>>
>> In this case the "pts" is always less than "now", hence there is no wait
>> and won't enter into the logic of  "EAGAIN return".
>> So no difference is observed.
>>
>> Here is another example in which "pts" may be larger than "now" 
>> before we
>> got the output:
>> https://docs.google.com/document/d/19fTcGMvTC6surQTvJPNNnngUrTEIRnqElu2lCCU6qwU/edit?usp=sharing 
>>
>
> Yeah, huge difference. Will adjust commit msg and push, since seeking 
> is not what's affected here.

New patch pushed as 6f206852289ee8997bef6a43a88252834d2d3e02

Thanks,
Gyan
Linjie Fu July 19, 2021, 12:14 p.m. UTC | #13
On Mon, Jul 19, 2021 at 3:32 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:

>
>
> On 2021-07-19 12:48, Gyan Doshi wrote:
> >
> >
> > On 2021-07-18 22:23, Linjie Fu wrote:
> >> On Sun, Jul 18, 2021 at 8:57 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >>>
> >>>
> >>> On 2021-07-18 15:37, Linjie Fu wrote:
> >>>> On Sun, Jul 18, 2021 at 5:26 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >>>>>
> >>>>> On 2021-07-18 13:35, Linjie Fu wrote:
> >>>>>> On Sun, Jul 18, 2021 at 1:21 PM Gyan Doshi <ffmpeg@gyani.pro>
> wrote:
> >>>>>>> On 2021-07-18 10:42, Linjie Fu wrote:
> >>>>>>>> Hi Gyan,
> >>>>>>>> On Sun, Jul 18, 2021 at 12:24 PM Gyan Doshi <ffmpeg@gyani.pro>
> >> wrote:
> >>>>>>>>> On 2021-07-18 09:32, Linjie Fu wrote:
> >>>>>>>>>> On Wed, Jul 7, 2021 at 9:42 AM Linjie Fu <
> >> linjie.justin.fu@gmail.com> wrote:
> >>>>>>>>>>> On Sun, Jul 4, 2021 at 10:50 PM Linjie Fu <fulinjie@zju.edu.cn
> >
> >> wrote:
> >>>>>>>>>>>> From: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Skip the logic of frame rate emulation until the input reaches
> >> the
> >>>>>>>>>>>> specified start time.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Test CMD:
> >>>>>>>>>>>>         $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f
> >>>>>>>>>>>> sdl2
> >> -
> >>>>>>>>>>>> Before the patch:
> >>>>>>>>>>>> first time to got frame, it takes 257305 us
> >>>>>>>>>>>> After this patch:
> >>>>>>>>>>>> first time to got frame, it takes 48879 us
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>> [v2]: fixed the mixed declaration and code warning
> >>>>>>>>>>>> Calculate the time to get the first frame:
> >>>>>>>>>>>>
> >>
> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
> >>
> >>>>>>>>>>>>       fftools/ffmpeg.c | 8 ++++++--
> >>>>>>>>>>>>       1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >>>>>>>>>>>> index e97d879cb3..c8849e4250 100644
> >>>>>>>>>>>> --- a/fftools/ffmpeg.c
> >>>>>>>>>>>> +++ b/fftools/ffmpeg.c
> >>>>>>>>>>>> @@ -4221,10 +4221,14 @@ static int get_input_packet(InputFile
> >> *f, AVPacket **pkt)
> >>>>>>>>>>>>       {
> >>>>>>>>>>>>           if (f->rate_emu) {
> >>>>>>>>>>>>               int i;
> >>>>>>>>>>>> +        int64_t pts;
> >>>>>>>>>>>> +        int64_t now;
> >>>>>>>>>>>>               for (i = 0; i < f->nb_streams; i++) {
> >>>>>>>>>>>>                   InputStream *ist =
> >>>>>>>>>>>> input_streams[f->ist_index
> >> + i];
> >>>>>>>>>>>> -            int64_t pts = av_rescale(ist->dts, 1000000,
> >> AV_TIME_BASE);
> >>>>>>>>>>>> -            int64_t now = av_gettime_relative() - ist->start;
> >>>>>>>>>>>> +            if (!ist->got_output)
> >>>>>>>>>>>> +                continue;
> >>>>>>>>>>>> +            pts = av_rescale(ist->dts, 1000000,
> >>>>>>>>>>>> AV_TIME_BASE);
> >>>>>>>>>>>> +            now = av_gettime_relative() - ist->start;
> >>>>>>>>>>>>                   if (pts > now)
> >>>>>>>>>>>>                       return AVERROR(EAGAIN);
> >>>>>>>>>>>>               }
> >>>>>>>>>>>> --
> >>>>>>>>>>>> 2.31.1
> >>>>>>>>>>> ping, thx.
> >>>>>>>>>>>
> >>>>>>>>>> Another ping, thx.
> >>>>>>>>> I pushed changes to this code yesterday. I don't think it's
> >> required
> >>>>>>>>> anymore, but do test.
> >>>>>>>>>
> >>>>>>>> Thanks for the review, tested after applying the readrate
> >>>>>>>> patch, I'm
> >>>>>>>> afraid that it's not identical as hope,
> >>>>>>>> since ist->nb_packets would increase no matter input stream got
> >> output or not:
> >>>>>>>> (lldb) p ist->nb_packets
> >>>>>>>> (uint64_t) $4 = 1
> >>>>>>>>
> >>>>>>>> (lldb) p ist->got_output
> >>>>>>>> (int) $5 = 0
> >>>>>>>>
> >>>>>>>> Hence we still need to add the check for ist->got_output, or
> >> replace
> >>>>>>>> the ist->nb_packets.
> >>>>>>> No, test the speed, not the parity of got_output. got_output is
> >>>>>>> only
> >>>>>>> incremented when the stream is decoded.
> >>>>>> The time interval consumed to get the first frame is tested as well,
> >>>>>> it's not speeded up:
> >>>>>> First time to get the frame, it takes 257395 us.
> >>>>>> After adding the check for got_output in decoding scenery, this
> >>>>>> would
> >>>>>> reduce to 48792 us.
> >>>>>>
> >>>>>> Hence it doesn't reduce the latency under decode scenery.
> >>>>>>
> >>>>>>> Won't work with streamcopy.
> >>>>>> Got your point.
> >>>>>>
> >>>>>>> nb_packets is the correct check since it's incremented after the
> >> initial
> >>>>>>> packet is demuxed.
> >>>>>> IIRC, the initial packet is demuxed right after the first time we
> >>>>>> call
> >>>>>> get_input_packet()-> av_read_frame(),  and nb_packets would
> >>>>>> increase.
> >>>>>> Hence, the check for ist->nb_packets would reduce the latency for
> >>>>>> demuxing the first packet,
> >>>>>> and won't help much if we use start_time like "-ss 30". Under the
> >>>>>> decoding scenery, we still
> >>>>>> need to wait until the pkt->pts specified time then demux the
> >>>>>> packet.
> >>>>>>
> >>>>>> If no objections, consider to change the check to:
> >>>>>>
> >>>>>> if (!ist->nb_packets || (!ist->got_output && ist->decoding_needed))
> >> continue;
> >>>>>> to reduce the waiting time for:
> >>>>>> 1. demuxing the first packet (if any)
> >>>>>> 2. demuxing the packet before we got the first decoded output.
> >>>>> Which sample file are you using, and how do I reproduce the timings?
> >>>>>
> >>>> It could be reproduced with a random input file (if long enough), and
> >>>> the time calculating method I used previously (needs to rebase a
> >>>> little bit for now):
> >>>>
> >>
> https://github.com/fulinjie/ffmpeg/commit/2aa4762e1e65709997b1ab9dd596332244db80ed
> >>
> >>>> CMD:
> >>>> $ffmpeg -re -ss 30 -i input.mp4 -pix_fmt yuv420p -f sdl2 -
> >>>> or
> >>>> $ffmpeg -re -ss 30 -i input.mp4 -f null -
> >>> I don't see a difference. See attached text file.
> >>>
> >> Checked the attached logs with decoding : ffmpeg -re -ss 30 -i
> >> sintel_trailer-720p.mp4 -an -vframes 24 -f null - -v 24
> >>
> >> #0:0 |nb_packets: 14 | pts: -2125000 |now: 10757 |decoded: 1
> >> |got_output: 0
> >> #0:0 |nb_packets: 15 | pts: -2041666 |now: 12543 |decoded: 1
> >> |got_output: 1
> >>     <-- time to obtain first frame, after 15 pkts = 12543 us
> >>
> >> In this case the "pts" is always less than "now", hence there is no wait
> >> and won't enter into the logic of  "EAGAIN return".
> >> So no difference is observed.
> >>
> >> Here is another example in which "pts" may be larger than "now"
> >> before we
> >> got the output:
> >>
> https://docs.google.com/document/d/19fTcGMvTC6surQTvJPNNnngUrTEIRnqElu2lCCU6qwU/edit?usp=sharing
> >>
> >
> > Yeah, huge difference. Will adjust commit msg and push, since seeking
> > is not what's affected here.
>
> New patch pushed as 6f206852289ee8997bef6a43a88252834d2d3e02
>

Thanks for the effort.

- linjie
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index e97d879cb3..c8849e4250 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -4221,10 +4221,14 @@  static int get_input_packet(InputFile *f, AVPacket **pkt)
 {
     if (f->rate_emu) {
         int i;
+        int64_t pts;
+        int64_t now;
         for (i = 0; i < f->nb_streams; i++) {
             InputStream *ist = input_streams[f->ist_index + i];
-            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
-            int64_t now = av_gettime_relative() - ist->start;
+            if (!ist->got_output)
+                continue;
+            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
+            now = av_gettime_relative() - ist->start;
             if (pts > now)
                 return AVERROR(EAGAIN);
         }