diff mbox series

[FFmpeg-devel,2/2] avformat/aviobuf: prefer to use avio_read_partial()

Message ID 1638507433-5005-2-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avformat/aviobuf: check if read_packet() exist before read_packet_wrapper() | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Limin Wang Dec. 3, 2021, 4:57 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

This is allowed to read fewer bytes than requested. The missing bytes can
be read in the next call.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/aviobuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marton Balint Dec. 3, 2021, 9:47 a.m. UTC | #1
On Fri, 3 Dec 2021, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> This is allowed to read fewer bytes than requested. The missing bytes can
> be read in the next call.

I don't think this is needed, after all we want to read all data, there is 
no point in reading it in smaller chunks, no?

Thanks,
Marton

>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> libavformat/aviobuf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 14688a2..ebb86e5 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -1317,7 +1317,7 @@ int avio_read_to_bprint(AVIOContext *h, AVBPrint *pb, size_t max_size)
>     int ret;
>     char buf[1024];
>     while (max_size) {
> -        ret = avio_read(h, buf, FFMIN(max_size, sizeof(buf)));
> +        ret = avio_read_partial(h, buf, FFMIN(max_size, sizeof(buf)));
>         if (ret == AVERROR_EOF)
>             return 0;
>         if (ret <= 0)
> -- 
> 1.8.3.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Limin Wang Dec. 3, 2021, 10:31 a.m. UTC | #2
On Fri, Dec 03, 2021 at 10:47:00AM +0100, Marton Balint wrote:
> 
> 
> On Fri, 3 Dec 2021, lance.lmwang@gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > This is allowed to read fewer bytes than requested. The missing bytes can
> > be read in the next call.
> 
> I don't think this is needed, after all we want to read all data, there is
> no point in reading it in smaller chunks, no?

Now the buf is 1024, in case I'm using sdp file, most of my simple testing sdp is about 300
byte, so I think it's preferable to use avio_read_partial and let it return directly. as we
don't expect to read 1024 at all.

> 
> Thanks,
> Marton
> 
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> > libavformat/aviobuf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > index 14688a2..ebb86e5 100644
> > --- a/libavformat/aviobuf.c
> > +++ b/libavformat/aviobuf.c
> > @@ -1317,7 +1317,7 @@ int avio_read_to_bprint(AVIOContext *h, AVBPrint *pb, size_t max_size)
> >     int ret;
> >     char buf[1024];
> >     while (max_size) {
> > -        ret = avio_read(h, buf, FFMIN(max_size, sizeof(buf)));
> > +        ret = avio_read_partial(h, buf, FFMIN(max_size, sizeof(buf)));
> >         if (ret == AVERROR_EOF)
> >             return 0;
> >         if (ret <= 0)
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint Dec. 3, 2021, 10:56 a.m. UTC | #3
On Fri, 3 Dec 2021, lance.lmwang@gmail.com wrote:

> On Fri, Dec 03, 2021 at 10:47:00AM +0100, Marton Balint wrote:
>>
>>
>> On Fri, 3 Dec 2021, lance.lmwang@gmail.com wrote:
>>
>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>
>>> This is allowed to read fewer bytes than requested. The missing bytes can
>>> be read in the next call.
>>
>> I don't think this is needed, after all we want to read all data, there is
>> no point in reading it in smaller chunks, no?
>
> Now the buf is 1024, in case I'm using sdp file, most of my simple testing sdp is about 300
> byte, so I think it's preferable to use avio_read_partial and let it return directly. as we
> don't expect to read 1024 at all.

I still don't get it. 1024 is the max you will read, nothing will happen 
if the file or the underlying buffer is less than that, so I don't 
understand when you write you want to read it "directly", or what benefit 
it might hold.

avio_read_partial() can be used to reduce latency, but since you want to 
read fixed amount of bytes in avio_read_to_bprint, there is no reason to 
use that.

Regards,
Marton

>
>>
>> Thanks,
>> Marton
>>
>>>
>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>> ---
>>> libavformat/aviobuf.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>> index 14688a2..ebb86e5 100644
>>> --- a/libavformat/aviobuf.c
>>> +++ b/libavformat/aviobuf.c
>>> @@ -1317,7 +1317,7 @@ int avio_read_to_bprint(AVIOContext *h, AVBPrint *pb, size_t max_size)
>>>     int ret;
>>>     char buf[1024];
>>>     while (max_size) {
>>> -        ret = avio_read(h, buf, FFMIN(max_size, sizeof(buf)));
>>> +        ret = avio_read_partial(h, buf, FFMIN(max_size, sizeof(buf)));
>>>         if (ret == AVERROR_EOF)
>>>             return 0;
>>>         if (ret <= 0)
>>> --
>>> 1.8.3.1
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> -- 
> Thanks,
> Limin Wang
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Martin Storsjö Dec. 3, 2021, 11:59 a.m. UTC | #4
On Fri, 3 Dec 2021, lance.lmwang@gmail.com wrote:

> On Fri, Dec 03, 2021 at 10:47:00AM +0100, Marton Balint wrote:
>>
>>
>> On Fri, 3 Dec 2021, lance.lmwang@gmail.com wrote:
>>
>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>
>>> This is allowed to read fewer bytes than requested. The missing bytes can
>>> be read in the next call.
>>
>> I don't think this is needed, after all we want to read all data, there is
>> no point in reading it in smaller chunks, no?
>
> Now the buf is 1024, in case I'm using sdp file, most of my simple 
> testing sdp is about 300 byte, so I think it's preferable to use 
> avio_read_partial and let it return directly. as we don't expect to read 
> 1024 at all.

No, that's not how it works.

avio_read_to_bprint loops until it either has read max_size bytes, or 
until it reaches EOF. If you use avio_read_partial, it would only loop 
more times, but it would take the exact same amount of time.

If you read a file from disk, once it has read the 300 bytes of actual 
data, and reach EOF, it returns directly.

avio_read_partial is only relevant for reading data e.g. over a network, 
where there's more data following, but you want to consume and process the 
initially received data before waiting for more. But with 
avio_read_partial it doesn't return until all data has been received 
anyway.

// Martin
Limin Wang Dec. 3, 2021, 12:50 p.m. UTC | #5
On Fri, Dec 03, 2021 at 01:59:16PM +0200, Martin Storsjö wrote:
> On Fri, 3 Dec 2021, lance.lmwang@gmail.com wrote:
> 
> > On Fri, Dec 03, 2021 at 10:47:00AM +0100, Marton Balint wrote:
> > > 
> > > 
> > > On Fri, 3 Dec 2021, lance.lmwang@gmail.com wrote:
> > > 
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > 
> > > > This is allowed to read fewer bytes than requested. The missing bytes can
> > > > be read in the next call.
> > > 
> > > I don't think this is needed, after all we want to read all data, there is
> > > no point in reading it in smaller chunks, no?
> > 
> > Now the buf is 1024, in case I'm using sdp file, most of my simple
> > testing sdp is about 300 byte, so I think it's preferable to use
> > avio_read_partial and let it return directly. as we don't expect to read
> > 1024 at all.
> 
> No, that's not how it works.
> 
> avio_read_to_bprint loops until it either has read max_size bytes, or until
> it reaches EOF. If you use avio_read_partial, it would only loop more times,
> but it would take the exact same amount of time.
> 
> If you read a file from disk, once it has read the 300 bytes of actual data,
> and reach EOF, it returns directly.
> 
> avio_read_partial is only relevant for reading data e.g. over a network,
> where there's more data following, but you want to consume and process the
> initially received data before waiting for more. But with avio_read_partial
> it doesn't return until all data has been received anyway.

Thanks for the comments, it seems that I misunderstand the usage of 
avio_read_partial, please ignore the patch then.

> 
> // Martin
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 14688a2..ebb86e5 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -1317,7 +1317,7 @@  int avio_read_to_bprint(AVIOContext *h, AVBPrint *pb, size_t max_size)
     int ret;
     char buf[1024];
     while (max_size) {
-        ret = avio_read(h, buf, FFMIN(max_size, sizeof(buf)));
+        ret = avio_read_partial(h, buf, FFMIN(max_size, sizeof(buf)));
         if (ret == AVERROR_EOF)
             return 0;
         if (ret <= 0)