Le dim. 4 mai 2025 à 17:23, Michael Niedermayer
<mich...@niedermayer.cc> a écrit :
>
> On Sun, May 04, 2025 at 11:42:13AM -0500, Romain Beauxis wrote:
> > Le dim. 4 mai 2025 à 09:04, Michael Niedermayer <mich...@niedermayer.cc>
a
> > écrit :
> > >
> > > On Sat, May 03, 2025 at 12:03:28PM -0500, Romain Beauxis wrote:
> > > > ---
> > > > libavformat/oggdec.c | 22 ++++++++++++++--------
> > > > libavformat/oggdec.h | 6 ++++++
> > > > 2 files changed, 20 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > > index 5339fdd32c..9baf8040a9 100644
> > > > --- a/libavformat/oggdec.c
> > > > +++ b/libavformat/oggdec.c
> > > > @@ -605,20 +605,26 @@ static int ogg_packet(AVFormatContext *s, int
> > *sid, int *dstart, int *dsize,
> > > > } else {
> > > > os->pflags = 0;
> > > > os->pduration = 0;
> > > > +
> > > > + ret = 0;
> > > > if (os->codec && os->codec->packet) {
> > > > if ((ret = os->codec->packet(s, idx)) < 0) {
> > > > av_log(s, AV_LOG_ERROR, "Packet processing failed:
> > %s\n", av_err2str(ret));
> > > > return ret;
> > > > }
> > > > }
> > > > - if (sid)
> > > > - *sid = idx;
> > > > - if (dstart)
> > > > - *dstart = os->pstart;
> > > > - if (dsize)
> > > > - *dsize = os->psize;
> > > > - if (fpos)
> > > > - *fpos = os->sync_pos;
> > > > +
> > > > + if (!ret) {
> > > > + if (sid)
> > > > + *sid = idx;
> > > > + if (dstart)
> > > > + *dstart = os->pstart;
> > > > + if (dsize)
> > > > + *dsize = os->psize;
> > > > + if (fpos)
> > > > + *fpos = os->sync_pos;
> > > > + }
> > > > +
> > > > os->pstart += os->psize;
> > > > os->psize = 0;
> > > > if(os->pstart == os->bufpos)
> > > > diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
> > > > index 43df23f4cb..09f698f99a 100644
> > > > --- a/libavformat/oggdec.h
> > > > +++ b/libavformat/oggdec.h
> > > > @@ -38,6 +38,12 @@ struct ogg_codec {
> > > > * -1 if an error occurred or for unsupported stream
> > > > */
> > > > int (*header)(AVFormatContext *, int);
> > > > + /**
> > > > + * Attempt to process a packet as a data packet
> > > > + * @return 1 if the packet was a header from a chained
bitstream.
> > > > + * 0 if the packet was a regular data packet.
> > > > + * -1 if an error occurred or for unsupported stream
> > > > + */
> > > > int (*packet)(AVFormatContext *, int);
> > > > /**
> > > > * Translate a granule into a timestamp.
> > >
> > > Iam still confused by this
> > >
> > > If this changes the API for ogg_codec.packet()
> > > and in the same patch theres a change to ogg_packet() which uses
> > > ogg_codec.packet()
> > >
> > > but then there is a 2nd patch that actually changes the
implementations
> > > of ogg_codec.packet() :
> > >
> > > so after the first patch, the API documentation is not correct,
> > > I thought that documentation and implementation change would happen
> > > in the same patch and the use of the more refined API would then be in
> > > a 2nd patch
> > > am i missing something here ?
> >
> > I must have misinterpreted your previous message:
> >
> > > Do you think this would merit a seperate patch ?
> > > I mean patch #1 changing the packet() return value and clearly stating
> > > that in the commit message
> > > and patch #2 using the new value ?
> >
> > Looks like you meant to do the reverse of what I sent, let me make it
more
> > clear:
> >
> > Order #1:
> > 1. Change the ogg_codec.packet() implementations to return 1 when
skipping
> > should occur
> > 2. Consume that value in ogg_packet and implement the actual skip in the
> > ogg bitstream
> >
> > Order #2:
> > 1. Introduce a new API functionality in ogg_packet that is opt-in, i.e.
> > returning 1 in the ogg packet function to optionally skip a packet from
the
> > ogg bitstream. API is available but not used yet.
> > 2. Implement the functionality in all the different ogg_codec.packet()
> > codec-specific files to skip header packets.
> >
> > Order #2 is what I sent. To me it makes more sense. One of the reasons
for
> > this order is that, also, the test diff would be colocated with the
change
> > in the atual codec-specific code. If you do order #1, the test diff
would
> > occur in the generic changes to ogg_packet, making it harder to track
where
> > the code that actually implements the test output changes comes from.
>
> If you prefer order #2 then i think, it could be done like this:
> (what bothers me is that API doc missmatching the actual implementation)
>
> Patch #1 docs only
> /**
> * Attempt to process a packet as a data packet
> * @return < 0 (AVERROR) code or -1 on error
> * ==0 if the packet was a regular data packet.
> * ==0 or 1 if the packet was a header from a chained
bitstream.
> */
>
> Patch #2
> ogg_packet() changes
> and
> * Attempt to process a packet as a data packet
> * @return < 0 (AVERROR) code or -1 on error
> * ==0 if the packet was a regular data packet.
> * ==0 or 1 if the packet was a header from a chained
bitstream.
> + * (1 will cause the packet to be skiped in calling code
(ogg_packet())
> */
>
> Patch #3
> oggparseopus.c changes and corresponding fate change
>
> Patch #4
> oggparsevorbis.c changes and corresponding fate change
>
> ...
>
> Patch #6
> * ==0 if the packet was a regular data packet.
> - * ==0 or 1 if the packet was a header from a chained
bitstream.
> - * (1 will cause the packet to be skiped in calling code
(ogg_packet())
> + * ==1 if the packet was a header from a chained bitstream.
> + * This will cause the packet to be skiped in calling
code (ogg_packet()
> */
>
>
> >
> > At the end of the day, I'm happy to commit these changes in any shape or
> > form so we can move forward with the rest of this work.
>
> I like to ensure the changes are understandable on a first glance, and
the whole logic
> in ogg* is a bit confusing with all the *packet stuff
Sounds good. Just sent the updated patchset. Hopefully that one will be
acceptable.
Le dim. 4 mai 2025 à 17:23, Michael Niedermayer <mich...@niedermayer.cc> a
écrit :
> On Sun, May 04, 2025 at 11:42:13AM -0500, Romain Beauxis wrote:
> > Le dim. 4 mai 2025 à 09:04, Michael Niedermayer <mich...@niedermayer.cc>
> a
> > écrit :
> > >
> > > On Sat, May 03, 2025 at 12:03:28PM -0500, Romain Beauxis wrote:
> > > > ---
> > > > libavformat/oggdec.c | 22 ++++++++++++++--------
> > > > libavformat/oggdec.h | 6 ++++++
> > > > 2 files changed, 20 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > > index 5339fdd32c..9baf8040a9 100644
> > > > --- a/libavformat/oggdec.c
> > > > +++ b/libavformat/oggdec.c
> > > > @@ -605,20 +605,26 @@ static int ogg_packet(AVFormatContext *s, int
> > *sid, int *dstart, int *dsize,
> > > > } else {
> > > > os->pflags = 0;
> > > > os->pduration = 0;
> > > > +
> > > > + ret = 0;
> > > > if (os->codec && os->codec->packet) {
> > > > if ((ret = os->codec->packet(s, idx)) < 0) {
> > > > av_log(s, AV_LOG_ERROR, "Packet processing failed:
> > %s\n", av_err2str(ret));
> > > > return ret;
> > > > }
> > > > }
> > > > - if (sid)
> > > > - *sid = idx;
> > > > - if (dstart)
> > > > - *dstart = os->pstart;
> > > > - if (dsize)
> > > > - *dsize = os->psize;
> > > > - if (fpos)
> > > > - *fpos = os->sync_pos;
> > > > +
> > > > + if (!ret) {
> > > > + if (sid)
> > > > + *sid = idx;
> > > > + if (dstart)
> > > > + *dstart = os->pstart;
> > > > + if (dsize)
> > > > + *dsize = os->psize;
> > > > + if (fpos)
> > > > + *fpos = os->sync_pos;
> > > > + }
> > > > +
> > > > os->pstart += os->psize;
> > > > os->psize = 0;
> > > > if(os->pstart == os->bufpos)
> > > > diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
> > > > index 43df23f4cb..09f698f99a 100644
> > > > --- a/libavformat/oggdec.h
> > > > +++ b/libavformat/oggdec.h
> > > > @@ -38,6 +38,12 @@ struct ogg_codec {
> > > > * -1 if an error occurred or for unsupported stream
> > > > */
> > > > int (*header)(AVFormatContext *, int);
> > > > + /**
> > > > + * Attempt to process a packet as a data packet
> > > > + * @return 1 if the packet was a header from a chained
> bitstream.
> > > > + * 0 if the packet was a regular data packet.
> > > > + * -1 if an error occurred or for unsupported stream
> > > > + */
> > > > int (*packet)(AVFormatContext *, int);
> > > > /**
> > > > * Translate a granule into a timestamp.
> > >
> > > Iam still confused by this
> > >
> > > If this changes the API for ogg_codec.packet()
> > > and in the same patch theres a change to ogg_packet() which uses
> > > ogg_codec.packet()
> > >
> > > but then there is a 2nd patch that actually changes the implementations
> > > of ogg_codec.packet() :
> > >
> > > so after the first patch, the API documentation is not correct,
> > > I thought that documentation and implementation change would happen
> > > in the same patch and the use of the more refined API would then be in
> > > a 2nd patch
> > > am i missing something here ?
> >
> > I must have misinterpreted your previous message:
> >
> > > Do you think this would merit a seperate patch ?
> > > I mean patch #1 changing the packet() return value and clearly stating
> > > that in the commit message
> > > and patch #2 using the new value ?
> >
> > Looks like you meant to do the reverse of what I sent, let me make it
> more
> > clear:
> >
> > Order #1:
> > 1. Change the ogg_codec.packet() implementations to return 1 when
> skipping
> > should occur
> > 2. Consume that value in ogg_packet and implement the actual skip in the
> > ogg bitstream
> >
> > Order #2:
> > 1. Introduce a new API functionality in ogg_packet that is opt-in, i.e.
> > returning 1 in the ogg packet function to optionally skip a packet from
> the
> > ogg bitstream. API is available but not used yet.
> > 2. Implement the functionality in all the different ogg_codec.packet()
> > codec-specific files to skip header packets.
> >
> > Order #2 is what I sent. To me it makes more sense. One of the reasons
> for
> > this order is that, also, the test diff would be colocated with the
> change
> > in the atual codec-specific code. If you do order #1, the test diff would
> > occur in the generic changes to ogg_packet, making it harder to track
> where
> > the code that actually implements the test output changes comes from.
>
> If you prefer order #2 then i think, it could be done like this:
> (what bothers me is that API doc missmatching the actual implementation)
>
> Patch #1 docs only
> /**
> * Attempt to process a packet as a data packet
> * @return < 0 (AVERROR) code or -1 on error
> * ==0 if the packet was a regular data packet.
> * ==0 or 1 if the packet was a header from a chained
> bitstream.
> */
>
> Patch #2
> ogg_packet() changes
> and
> * Attempt to process a packet as a data packet
> * @return < 0 (AVERROR) code or -1 on error
> * ==0 if the packet was a regular data packet.
> * ==0 or 1 if the packet was a header from a chained
> bitstream.
> + * (1 will cause the packet to be skiped in calling code
> (ogg_packet())
> */
>
> Patch #3
> oggparseopus.c changes and corresponding fate change
>
> Patch #4
> oggparsevorbis.c changes and corresponding fate change
>
> ...
>
> Patch #6
> * ==0 if the packet was a regular data packet.
> - * ==0 or 1 if the packet was a header from a chained
> bitstream.
> - * (1 will cause the packet to be skiped in calling code
> (ogg_packet())
> + * ==1 if the packet was a header from a chained bitstream.
> + * This will cause the packet to be skiped in calling code
> (ogg_packet()
> */
>
>
> >
> > At the end of the day, I'm happy to commit these changes in any shape or
> > form so we can move forward with the rest of this work.
>
> I like to ensure the changes are understandable on a first glance, and the
> whole logic
> in ogg* is a bit confusing with all the *packet stuff
>
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> During times of universal deceit, telling the truth becomes a
> revolutionary act. -- George Orwell
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
