diff mbox series

[FFmpeg-devel] avformat/avio{, buf}: introduce public AVIOContext::bytes_read

Message ID 20210926154818.15595-1-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/avio{, buf}: introduce public AVIOContext::bytes_read | 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

Jan Ekström Sept. 26, 2021, 3:48 p.m. UTC
Such a field can be seen as generally useful in cases where the
API user is not implementing custom AVIO callbacks, but still would
like to know if data is being read even if AVPackets are not being
returned.
---
Originally I thought about making an accessor for the private field, to
not grow the public struct's size (and have a duplicate field, as well
as making sure the value was read-only). But an objection was raised
that such accessors should be refrained from as they unnecessarily
filled the function symbol space or so. Together with the objection, a
proposal of making it a field on the public struct that was only written
to was proposed.

This patch follows that proposal. 

 doc/APIchanges        | 3 +++
 libavformat/avio.h    | 5 +++++
 libavformat/aviobuf.c | 2 ++
 libavformat/version.h | 2 +-
 4 files changed, 11 insertions(+), 1 deletion(-)

Comments

Jan Ekström Oct. 1, 2021, 6:01 p.m. UTC | #1
On Sun, Sep 26, 2021 at 6:48 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> Such a field can be seen as generally useful in cases where the
> API user is not implementing custom AVIO callbacks, but still would
> like to know if data is being read even if AVPackets are not being
> returned.
> ---

Ping.

Jan
Michael Niedermayer Oct. 2, 2021, 10:32 a.m. UTC | #2
On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> Such a field can be seen as generally useful in cases where the
> API user is not implementing custom AVIO callbacks, but still would
> like to know if data is being read even if AVPackets are not being
> returned.
> ---
> Originally I thought about making an accessor for the private field, to
> not grow the public struct's size (and have a duplicate field, as well
> as making sure the value was read-only). But an objection was raised
> that such accessors should be refrained from as they unnecessarily
> filled the function symbol space or so. Together with the objection, a
> proposal of making it a field on the public struct that was only written
> to was proposed.
> 
> This patch follows that proposal. 
> 
>  doc/APIchanges        | 3 +++
>  libavformat/avio.h    | 5 +++++
>  libavformat/aviobuf.c | 2 ++
>  libavformat/version.h | 2 +-
>  4 files changed, 11 insertions(+), 1 deletion(-)

There are 3 statistics, read, write and seek
shouldnt all 3 be provided to the user?

thx

[...]
Jan Ekström Oct. 2, 2021, 11:42 a.m. UTC | #3
On Sat, Oct 2, 2021 at 1:32 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> > Such a field can be seen as generally useful in cases where the
> > API user is not implementing custom AVIO callbacks, but still would
> > like to know if data is being read even if AVPackets are not being
> > returned.
> > ---
> > Originally I thought about making an accessor for the private field, to
> > not grow the public struct's size (and have a duplicate field, as well
> > as making sure the value was read-only). But an objection was raised
> > that such accessors should be refrained from as they unnecessarily
> > filled the function symbol space or so. Together with the objection, a
> > proposal of making it a field on the public struct that was only written
> > to was proposed.
> >
> > This patch follows that proposal.
> >
> >  doc/APIchanges        | 3 +++
> >  libavformat/avio.h    | 5 +++++
> >  libavformat/aviobuf.c | 2 ++
> >  libavformat/version.h | 2 +-
> >  4 files changed, 11 insertions(+), 1 deletion(-)
>
> There are 3 statistics, read, write and seek
> shouldnt all 3 be provided to the user?
>
> thx
>

I added one which I have seen actually utilized by at least one API
client, and then others could be added as per responses.

That is why I pinged, as I had not received any responses - either
positive or negative.

Writing I can see a use for, seek I am not as sure of. But if you
believe all of them should be exposed I am fine with that.

Jan
Michael Niedermayer Oct. 2, 2021, 11:51 a.m. UTC | #4
On Sat, Oct 02, 2021 at 02:42:52PM +0300, Jan Ekström wrote:
> On Sat, Oct 2, 2021 at 1:32 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> > > Such a field can be seen as generally useful in cases where the
> > > API user is not implementing custom AVIO callbacks, but still would
> > > like to know if data is being read even if AVPackets are not being
> > > returned.
> > > ---
> > > Originally I thought about making an accessor for the private field, to
> > > not grow the public struct's size (and have a duplicate field, as well
> > > as making sure the value was read-only). But an objection was raised
> > > that such accessors should be refrained from as they unnecessarily
> > > filled the function symbol space or so. Together with the objection, a
> > > proposal of making it a field on the public struct that was only written
> > > to was proposed.
> > >
> > > This patch follows that proposal.
> > >
> > >  doc/APIchanges        | 3 +++
> > >  libavformat/avio.h    | 5 +++++
> > >  libavformat/aviobuf.c | 2 ++
> > >  libavformat/version.h | 2 +-
> > >  4 files changed, 11 insertions(+), 1 deletion(-)
> >
> > There are 3 statistics, read, write and seek
> > shouldnt all 3 be provided to the user?
> >
> > thx
> >
> 
> I added one which I have seen actually utilized by at least one API
> client, and then others could be added as per responses.
> 
> That is why I pinged, as I had not received any responses - either
> positive or negative.
> 

> Writing I can see a use for, seek I am not as sure of. But if you
> believe all of them should be exposed I am fine with that.

seek is timeconsuming especially if its over a network due to
latency.
So for example if suddenly the number of seeks changes that
could be interresting. 

thx

[...]
Jan Ekström Oct. 3, 2021, 9:25 p.m. UTC | #5
On Sat, Oct 2, 2021 at 2:51 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sat, Oct 02, 2021 at 02:42:52PM +0300, Jan Ekström wrote:
> > On Sat, Oct 2, 2021 at 1:32 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> > > > Such a field can be seen as generally useful in cases where the
> > > > API user is not implementing custom AVIO callbacks, but still would
> > > > like to know if data is being read even if AVPackets are not being
> > > > returned.
> > > > ---
> > > > Originally I thought about making an accessor for the private field, to
> > > > not grow the public struct's size (and have a duplicate field, as well
> > > > as making sure the value was read-only). But an objection was raised
> > > > that such accessors should be refrained from as they unnecessarily
> > > > filled the function symbol space or so. Together with the objection, a
> > > > proposal of making it a field on the public struct that was only written
> > > > to was proposed.
> > > >
> > > > This patch follows that proposal.
> > > >
> > > >  doc/APIchanges        | 3 +++
> > > >  libavformat/avio.h    | 5 +++++
> > > >  libavformat/aviobuf.c | 2 ++
> > > >  libavformat/version.h | 2 +-
> > > >  4 files changed, 11 insertions(+), 1 deletion(-)
> > >
> > > There are 3 statistics, read, write and seek
> > > shouldnt all 3 be provided to the user?
> > >
> > > thx
> > >
> >
> > I added one which I have seen actually utilized by at least one API
> > client, and then others could be added as per responses.
> >
> > That is why I pinged, as I had not received any responses - either
> > positive or negative.
> >
>
> > Writing I can see a use for, seek I am not as sure of. But if you
> > believe all of them should be exposed I am fine with that.
>
> seek is timeconsuming especially if its over a network due to
> latency.
> So for example if suddenly the number of seeks changes that
> could be interresting.
>
> thx

I would prefer to add fields which were noted as specifically private
and then cleaned up when there are actual API client users that would
see them as useful, or if there are clear use cases where they'd be
useful. I have seen the read bytes statistic actually being utilized
by an API client with a comment:

// This is fully intentional - there is no other way to get this
// information (not even by custom I/O, because the connection reuse
// mechanism by the HLS demuxer would get disabled)

(note: not sure if that any more holds true)

Also I double-checked and the write statistic was in counts, not
bytes. So that I see as generally less useful than something like
"bytes_written" if such were to exist.

Jan
Jan Ekström Oct. 3, 2021, 9:35 p.m. UTC | #6
On Mon, Oct 4, 2021 at 12:25 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Sat, Oct 2, 2021 at 2:51 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Sat, Oct 02, 2021 at 02:42:52PM +0300, Jan Ekström wrote:
> > > On Sat, Oct 2, 2021 at 1:32 PM Michael Niedermayer
> > > <michael@niedermayer.cc> wrote:
> > > >
> > > > On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> > > > > Such a field can be seen as generally useful in cases where the
> > > > > API user is not implementing custom AVIO callbacks, but still would
> > > > > like to know if data is being read even if AVPackets are not being
> > > > > returned.
> > > > > ---
> > > > > Originally I thought about making an accessor for the private field, to
> > > > > not grow the public struct's size (and have a duplicate field, as well
> > > > > as making sure the value was read-only). But an objection was raised
> > > > > that such accessors should be refrained from as they unnecessarily
> > > > > filled the function symbol space or so. Together with the objection, a
> > > > > proposal of making it a field on the public struct that was only written
> > > > > to was proposed.
> > > > >
> > > > > This patch follows that proposal.
> > > > >
> > > > >  doc/APIchanges        | 3 +++
> > > > >  libavformat/avio.h    | 5 +++++
> > > > >  libavformat/aviobuf.c | 2 ++
> > > > >  libavformat/version.h | 2 +-
> > > > >  4 files changed, 11 insertions(+), 1 deletion(-)
> > > >
> > > > There are 3 statistics, read, write and seek
> > > > shouldnt all 3 be provided to the user?
> > > >
> > > > thx
> > > >
> > >
> > > I added one which I have seen actually utilized by at least one API
> > > client, and then others could be added as per responses.
> > >
> > > That is why I pinged, as I had not received any responses - either
> > > positive or negative.
> > >
> >
> > > Writing I can see a use for, seek I am not as sure of. But if you
> > > believe all of them should be exposed I am fine with that.
> >
> > seek is timeconsuming especially if its over a network due to
> > latency.
> > So for example if suddenly the number of seeks changes that
> > could be interresting.
> >
> > thx
>
> I would prefer to add fields which were noted as specifically private
> and then cleaned up when there are actual API client users that would
> see them as useful, or if there are clear use cases where they'd be
> useful. I have seen the read bytes statistic actually being utilized
> by an API client with a comment:
>
> // This is fully intentional - there is no other way to get this
> // information (not even by custom I/O, because the connection reuse
> // mechanism by the HLS demuxer would get disabled)
>
> (note: not sure if that any more holds true)
>
> Also I double-checked and the write statistic was in counts, not
> bytes. So that I see as generally less useful than something like
> "bytes_written" if such were to exist.

Also just checked, and AVIOContext::written seems to be this
"bytes_written" already :) Just completely undocumented.

Jan
Michael Niedermayer Oct. 3, 2021, 10:06 p.m. UTC | #7
On Mon, Oct 04, 2021 at 12:25:26AM +0300, Jan Ekström wrote:
> On Sat, Oct 2, 2021 at 2:51 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Sat, Oct 02, 2021 at 02:42:52PM +0300, Jan Ekström wrote:
> > > On Sat, Oct 2, 2021 at 1:32 PM Michael Niedermayer
> > > <michael@niedermayer.cc> wrote:
> > > >
> > > > On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> > > > > Such a field can be seen as generally useful in cases where the
> > > > > API user is not implementing custom AVIO callbacks, but still would
> > > > > like to know if data is being read even if AVPackets are not being
> > > > > returned.
> > > > > ---
> > > > > Originally I thought about making an accessor for the private field, to
> > > > > not grow the public struct's size (and have a duplicate field, as well
> > > > > as making sure the value was read-only). But an objection was raised
> > > > > that such accessors should be refrained from as they unnecessarily
> > > > > filled the function symbol space or so. Together with the objection, a
> > > > > proposal of making it a field on the public struct that was only written
> > > > > to was proposed.
> > > > >
> > > > > This patch follows that proposal.
> > > > >
> > > > >  doc/APIchanges        | 3 +++
> > > > >  libavformat/avio.h    | 5 +++++
> > > > >  libavformat/aviobuf.c | 2 ++
> > > > >  libavformat/version.h | 2 +-
> > > > >  4 files changed, 11 insertions(+), 1 deletion(-)
> > > >
> > > > There are 3 statistics, read, write and seek
> > > > shouldnt all 3 be provided to the user?
> > > >
> > > > thx
> > > >
> > >
> > > I added one which I have seen actually utilized by at least one API
> > > client, and then others could be added as per responses.
> > >
> > > That is why I pinged, as I had not received any responses - either
> > > positive or negative.
> > >
> >
> > > Writing I can see a use for, seek I am not as sure of. But if you
> > > believe all of them should be exposed I am fine with that.
> >
> > seek is timeconsuming especially if its over a network due to
> > latency.
> > So for example if suddenly the number of seeks changes that
> > could be interresting.
> >
> > thx
> 
> I would prefer to add fields which were noted as specifically private
> and then cleaned up when there are actual API client users that would
> see them as useful, or if there are clear use cases where they'd be
> useful. I have seen the read bytes statistic actually being utilized
> by an API client with a comment:

Assume a network protocol, TCP, UDP, HTTP, RT*P whatever
how do you tune the buffer sizes ?
Can the number of seeks be used ?
or from a different point of view, if there are alot of seeks should
a user app try to increase the buffer sizes ?

maybe iam missing something but when playing a not perfectly interleaved file
over the network the buffer size should be what makes the difference between
that working or not working
ideally a user app shouldnt need to mess with this, of course and these values
should all be automagically adjusted

If a user app fails to get packets in realtime over the network, it would
fail to play that stream. Some user apps could display a warning message to
the user about it.
If now the user app has access to the number of seeks it could be more
specific in the warning to the user. 
"Unable to play network is maybe too slow"
"Unable to play buffer is maybe too small or file is poorly interleaved"
...

Maybe iam just seeing all this from the wrong side i dunno but to me it seems
usefull to a user app to have access to the number of seeks and these seem
non contrived use cases to me ... Ive gotten random point to nowhere
warnings about playback issues and restarting the computer obviously that
never was the issue.

thx

[...]
Jan Ekström Oct. 4, 2021, 9:12 a.m. UTC | #8
On Mon, Oct 4, 2021 at 1:06 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Mon, Oct 04, 2021 at 12:25:26AM +0300, Jan Ekström wrote:
> > On Sat, Oct 2, 2021 at 2:51 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Sat, Oct 02, 2021 at 02:42:52PM +0300, Jan Ekström wrote:
> > > > On Sat, Oct 2, 2021 at 1:32 PM Michael Niedermayer
> > > > <michael@niedermayer.cc> wrote:
> > > > >
> > > > > On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> > > > > > Such a field can be seen as generally useful in cases where the
> > > > > > API user is not implementing custom AVIO callbacks, but still would
> > > > > > like to know if data is being read even if AVPackets are not being
> > > > > > returned.
> > > > > > ---
> > > > > > Originally I thought about making an accessor for the private field, to
> > > > > > not grow the public struct's size (and have a duplicate field, as well
> > > > > > as making sure the value was read-only). But an objection was raised
> > > > > > that such accessors should be refrained from as they unnecessarily
> > > > > > filled the function symbol space or so. Together with the objection, a
> > > > > > proposal of making it a field on the public struct that was only written
> > > > > > to was proposed.
> > > > > >
> > > > > > This patch follows that proposal.
> > > > > >
> > > > > >  doc/APIchanges        | 3 +++
> > > > > >  libavformat/avio.h    | 5 +++++
> > > > > >  libavformat/aviobuf.c | 2 ++
> > > > > >  libavformat/version.h | 2 +-
> > > > > >  4 files changed, 11 insertions(+), 1 deletion(-)
> > > > >
> > > > > There are 3 statistics, read, write and seek
> > > > > shouldnt all 3 be provided to the user?
> > > > >
> > > > > thx
> > > > >
> > > >
> > > > I added one which I have seen actually utilized by at least one API
> > > > client, and then others could be added as per responses.
> > > >
> > > > That is why I pinged, as I had not received any responses - either
> > > > positive or negative.
> > > >
> > >
> > > > Writing I can see a use for, seek I am not as sure of. But if you
> > > > believe all of them should be exposed I am fine with that.
> > >
> > > seek is timeconsuming especially if its over a network due to
> > > latency.
> > > So for example if suddenly the number of seeks changes that
> > > could be interresting.
> > >
> > > thx
> >
> > I would prefer to add fields which were noted as specifically private
> > and then cleaned up when there are actual API client users that would
> > see them as useful, or if there are clear use cases where they'd be
> > useful. I have seen the read bytes statistic actually being utilized
> > by an API client with a comment:
>
> Assume a network protocol, TCP, UDP, HTTP, RT*P whatever
> how do you tune the buffer sizes ?
> Can the number of seeks be used ?
> or from a different point of view, if there are alot of seeks should
> a user app try to increase the buffer sizes ?
>
> maybe iam missing something but when playing a not perfectly interleaved file
> over the network the buffer size should be what makes the difference between
> that working or not working
> ideally a user app shouldnt need to mess with this, of course and these values
> should all be automagically adjusted
>
> If a user app fails to get packets in realtime over the network, it would
> fail to play that stream. Some user apps could display a warning message to
> the user about it.
> If now the user app has access to the number of seeks it could be more
> specific in the warning to the user.
> "Unable to play network is maybe too slow"
> "Unable to play buffer is maybe too small or file is poorly interleaved"
> ...
>
> Maybe iam just seeing all this from the wrong side i dunno but to me it seems
> usefull to a user app to have access to the number of seeks and these seem
> non contrived use cases to me ... Ive gotten random point to nowhere
> warnings about playback issues and restarting the computer obviously that
> never was the issue.
>
> thx
>

OK, I think this is now focusing on the wrong point, sorry.

I think it's just better for me to note that I am not the best person
to post (and thus be the one to argue for the usefulness in reviews if
someone asks why I am bringing those private entries that were once
removed back to the public struct) of those other entries.

Jan
Jan Ekström Oct. 11, 2021, 7:24 p.m. UTC | #9
On Mon, Oct 4, 2021 at 12:12 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Mon, Oct 4, 2021 at 1:06 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Mon, Oct 04, 2021 at 12:25:26AM +0300, Jan Ekström wrote:
> > > On Sat, Oct 2, 2021 at 2:51 PM Michael Niedermayer
> > > <michael@niedermayer.cc> wrote:
> > > >
> > > > On Sat, Oct 02, 2021 at 02:42:52PM +0300, Jan Ekström wrote:
> > > > > On Sat, Oct 2, 2021 at 1:32 PM Michael Niedermayer
> > > > > <michael@niedermayer.cc> wrote:
> > > > > >
> > > > > > On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> > > > > > > Such a field can be seen as generally useful in cases where the
> > > > > > > API user is not implementing custom AVIO callbacks, but still would
> > > > > > > like to know if data is being read even if AVPackets are not being
> > > > > > > returned.
> > > > > > > ---
> > > > > > > Originally I thought about making an accessor for the private field, to
> > > > > > > not grow the public struct's size (and have a duplicate field, as well
> > > > > > > as making sure the value was read-only). But an objection was raised
> > > > > > > that such accessors should be refrained from as they unnecessarily
> > > > > > > filled the function symbol space or so. Together with the objection, a
> > > > > > > proposal of making it a field on the public struct that was only written
> > > > > > > to was proposed.
> > > > > > >
> > > > > > > This patch follows that proposal.
> > > > > > >
> > > > > > >  doc/APIchanges        | 3 +++
> > > > > > >  libavformat/avio.h    | 5 +++++
> > > > > > >  libavformat/aviobuf.c | 2 ++
> > > > > > >  libavformat/version.h | 2 +-
> > > > > > >  4 files changed, 11 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > There are 3 statistics, read, write and seek
> > > > > > shouldnt all 3 be provided to the user?
> > > > > >
> > > > > > thx
> > > > > >
> > > > >
> > > > > I added one which I have seen actually utilized by at least one API
> > > > > client, and then others could be added as per responses.
> > > > >
> > > > > That is why I pinged, as I had not received any responses - either
> > > > > positive or negative.
> > > > >
> > > >
> > > > > Writing I can see a use for, seek I am not as sure of. But if you
> > > > > believe all of them should be exposed I am fine with that.
> > > >
> > > > seek is timeconsuming especially if its over a network due to
> > > > latency.
> > > > So for example if suddenly the number of seeks changes that
> > > > could be interresting.
> > > >
> > > > thx
> > >
> > > I would prefer to add fields which were noted as specifically private
> > > and then cleaned up when there are actual API client users that would
> > > see them as useful, or if there are clear use cases where they'd be
> > > useful. I have seen the read bytes statistic actually being utilized
> > > by an API client with a comment:
> >
> > Assume a network protocol, TCP, UDP, HTTP, RT*P whatever
> > how do you tune the buffer sizes ?
> > Can the number of seeks be used ?
> > or from a different point of view, if there are alot of seeks should
> > a user app try to increase the buffer sizes ?
> >
> > maybe iam missing something but when playing a not perfectly interleaved file
> > over the network the buffer size should be what makes the difference between
> > that working or not working
> > ideally a user app shouldnt need to mess with this, of course and these values
> > should all be automagically adjusted
> >
> > If a user app fails to get packets in realtime over the network, it would
> > fail to play that stream. Some user apps could display a warning message to
> > the user about it.
> > If now the user app has access to the number of seeks it could be more
> > specific in the warning to the user.
> > "Unable to play network is maybe too slow"
> > "Unable to play buffer is maybe too small or file is poorly interleaved"
> > ...
> >
> > Maybe iam just seeing all this from the wrong side i dunno but to me it seems
> > usefull to a user app to have access to the number of seeks and these seem
> > non contrived use cases to me ... Ive gotten random point to nowhere
> > warnings about playback issues and restarting the computer obviously that
> > never was the issue.
> >
> > thx
> >
>
> OK, I think this is now focusing on the wrong point, sorry.
>
> I think it's just better for me to note that I am not the best person
> to post (and thus be the one to argue for the usefulness in reviews if
> someone asks why I am bringing those private entries that were once
> removed back to the public struct) of those other entries.

Ping? Is this now in a purgatory state due to me not wanting to be the
one to argue for the other options in review?

Jan
Michael Niedermayer Oct. 12, 2021, 11:26 a.m. UTC | #10
On Mon, Oct 11, 2021 at 10:24:47PM +0300, Jan Ekström wrote:
> On Mon, Oct 4, 2021 at 12:12 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > On Mon, Oct 4, 2021 at 1:06 AM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Mon, Oct 04, 2021 at 12:25:26AM +0300, Jan Ekström wrote:
> > > > On Sat, Oct 2, 2021 at 2:51 PM Michael Niedermayer
> > > > <michael@niedermayer.cc> wrote:
> > > > >
> > > > > On Sat, Oct 02, 2021 at 02:42:52PM +0300, Jan Ekström wrote:
> > > > > > On Sat, Oct 2, 2021 at 1:32 PM Michael Niedermayer
> > > > > > <michael@niedermayer.cc> wrote:
> > > > > > >
> > > > > > > On Sun, Sep 26, 2021 at 06:48:18PM +0300, Jan Ekström wrote:
> > > > > > > > Such a field can be seen as generally useful in cases where the
> > > > > > > > API user is not implementing custom AVIO callbacks, but still would
> > > > > > > > like to know if data is being read even if AVPackets are not being
> > > > > > > > returned.
> > > > > > > > ---
> > > > > > > > Originally I thought about making an accessor for the private field, to
> > > > > > > > not grow the public struct's size (and have a duplicate field, as well
> > > > > > > > as making sure the value was read-only). But an objection was raised
> > > > > > > > that such accessors should be refrained from as they unnecessarily
> > > > > > > > filled the function symbol space or so. Together with the objection, a
> > > > > > > > proposal of making it a field on the public struct that was only written
> > > > > > > > to was proposed.
> > > > > > > >
> > > > > > > > This patch follows that proposal.
> > > > > > > >
> > > > > > > >  doc/APIchanges        | 3 +++
> > > > > > > >  libavformat/avio.h    | 5 +++++
> > > > > > > >  libavformat/aviobuf.c | 2 ++
> > > > > > > >  libavformat/version.h | 2 +-
> > > > > > > >  4 files changed, 11 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > There are 3 statistics, read, write and seek
> > > > > > > shouldnt all 3 be provided to the user?
> > > > > > >
> > > > > > > thx
> > > > > > >
> > > > > >
> > > > > > I added one which I have seen actually utilized by at least one API
> > > > > > client, and then others could be added as per responses.
> > > > > >
> > > > > > That is why I pinged, as I had not received any responses - either
> > > > > > positive or negative.
> > > > > >
> > > > >
> > > > > > Writing I can see a use for, seek I am not as sure of. But if you
> > > > > > believe all of them should be exposed I am fine with that.
> > > > >
> > > > > seek is timeconsuming especially if its over a network due to
> > > > > latency.
> > > > > So for example if suddenly the number of seeks changes that
> > > > > could be interresting.
> > > > >
> > > > > thx
> > > >
> > > > I would prefer to add fields which were noted as specifically private
> > > > and then cleaned up when there are actual API client users that would
> > > > see them as useful, or if there are clear use cases where they'd be
> > > > useful. I have seen the read bytes statistic actually being utilized
> > > > by an API client with a comment:
> > >
> > > Assume a network protocol, TCP, UDP, HTTP, RT*P whatever
> > > how do you tune the buffer sizes ?
> > > Can the number of seeks be used ?
> > > or from a different point of view, if there are alot of seeks should
> > > a user app try to increase the buffer sizes ?
> > >
> > > maybe iam missing something but when playing a not perfectly interleaved file
> > > over the network the buffer size should be what makes the difference between
> > > that working or not working
> > > ideally a user app shouldnt need to mess with this, of course and these values
> > > should all be automagically adjusted
> > >
> > > If a user app fails to get packets in realtime over the network, it would
> > > fail to play that stream. Some user apps could display a warning message to
> > > the user about it.
> > > If now the user app has access to the number of seeks it could be more
> > > specific in the warning to the user.
> > > "Unable to play network is maybe too slow"
> > > "Unable to play buffer is maybe too small or file is poorly interleaved"
> > > ...
> > >
> > > Maybe iam just seeing all this from the wrong side i dunno but to me it seems
> > > usefull to a user app to have access to the number of seeks and these seem
> > > non contrived use cases to me ... Ive gotten random point to nowhere
> > > warnings about playback issues and restarting the computer obviously that
> > > never was the issue.
> > >
> > > thx
> > >
> >
> > OK, I think this is now focusing on the wrong point, sorry.
> >
> > I think it's just better for me to note that I am not the best person
> > to post (and thus be the one to argue for the usefulness in reviews if
> > someone asks why I am bringing those private entries that were once
> > removed back to the public struct) of those other entries.
> 
> Ping? Is this now in a purgatory state due to me not wanting to be the
> one to argue for the other options in review?

Well, iam a bit undecided about what is best
a more broader IO statistic API that provides more details and that can be
extended and maybe can be disabled so as not to waste resources when the
statistics are not needed was a thought i had.
Simply adding one field because you need it, then the next person adds
one field he needs and so on. In the end the result is called a mess.
a few fields scattered over the structs and copied around between them
every time they change.

IIUC you are not interrested in implementing and arguing for a more
complete IO statistics API. And I have not enough time to implement
and argue for it myself ATM. There are no objections against your
patch so nothing stops you from applying it ...

thx

[...]
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index 7b267a79ac..6a8cf8ea15 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,9 @@  libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2021-09-26 - xxxxxxxxxx - lavf 59.6.100 - avio.h
+  Introduce a public bytes_read statistic field to AVIOContext.
+
 2021-09-21 - xxxxxxxxxx - lavu 57.7.100 - pixfmt.h
   Add AV_PIX_FMT_X2BGR10.
 
diff --git a/libavformat/avio.h b/libavformat/avio.h
index a7b56ab667..2cfb548231 100644
--- a/libavformat/avio.h
+++ b/libavformat/avio.h
@@ -297,6 +297,11 @@  typedef struct AVIOContext {
      * used keeping track of already written data for a later flush.
      */
     unsigned char *buf_ptr_max;
+
+    /**
+     * Read-only statistic of bytes read for this AVIOContext.
+     */
+    int64_t bytes_read;
 } AVIOContext;
 
 /**
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index d79e41ca77..33825ade73 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -572,6 +572,7 @@  static void fill_buffer(AVIOContext *s)
         s->buf_ptr = dst;
         s->buf_end = dst + len;
         ffiocontext(s)->bytes_read += len;
+        s->bytes_read = ffiocontext(s)->bytes_read;
     }
 }
 
@@ -645,6 +646,7 @@  int avio_read(AVIOContext *s, unsigned char *buf, int size)
                 } else {
                     s->pos += len;
                     ffiocontext(s)->bytes_read += len;
+                    s->bytes_read = ffiocontext(s)->bytes_read;
                     size -= len;
                     buf += len;
                     // reset the buffer
diff --git a/libavformat/version.h b/libavformat/version.h
index 13df244d97..d5dd22059b 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -32,7 +32,7 @@ 
 // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  59
-#define LIBAVFORMAT_VERSION_MINOR   5
+#define LIBAVFORMAT_VERSION_MINOR   6
 #define LIBAVFORMAT_VERSION_MICRO 100
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \