| Message ID | 20200508105240.15480-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/(null|opus)_bsf: Use ff_bsf_get_packet_ref() directly | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 5/8/2020 7:52 AM, Andreas Rheinhardt wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > The change to the documentation would be unnecessary if James' patch > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260984.html were > applied. But it is only internal documentation, so changing and > reverting it is easy. How does my patch change the success return values of ff_bsf_get_packet_ref()? > > libavcodec/bsf.h | 2 +- > libavcodec/null_bsf.c | 7 +------ > libavcodec/opus_metadata_bsf.c | 7 +------ > 3 files changed, 3 insertions(+), 13 deletions(-) > > diff --git a/libavcodec/bsf.h b/libavcodec/bsf.h > index af035eee44..d04f1d3068 100644 > --- a/libavcodec/bsf.h > +++ b/libavcodec/bsf.h > @@ -35,7 +35,7 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt); > * @param ctx pointer to AVBSFContext of filter > * @param pkt pointer to packet to move reference to > * > - * @return 0>= on success, negative AVERROR in case of failure > + * @return 0 on success, negative AVERROR in case of failure This change is ok, but I don't think it's relevant to this patch. > */ > int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt); > > diff --git a/libavcodec/null_bsf.c b/libavcodec/null_bsf.c > index 24d26dfb1a..37f4640c87 100644 > --- a/libavcodec/null_bsf.c > +++ b/libavcodec/null_bsf.c > @@ -24,12 +24,7 @@ > #include "avcodec.h" > #include "bsf.h" > > -static int null_filter(AVBSFContext *ctx, AVPacket *pkt) > -{ > - return ff_bsf_get_packet_ref(ctx, pkt); > -} > - > const AVBitStreamFilter ff_null_bsf = { > .name = "null", > - .filter = null_filter, > + .filter = ff_bsf_get_packet_ref, > }; > diff --git a/libavcodec/opus_metadata_bsf.c b/libavcodec/opus_metadata_bsf.c > index 867ad830d3..d22db54f30 100644 > --- a/libavcodec/opus_metadata_bsf.c > +++ b/libavcodec/opus_metadata_bsf.c > @@ -25,11 +25,6 @@ typedef struct OpusBSFContext { > int gain; > } OpusBSFContext; > > -static int opus_metadata_filter(AVBSFContext *bsfc, AVPacket *pkt) > -{ > - return ff_bsf_get_packet_ref(bsfc, pkt); > -} > - > static int opus_metadata_init(AVBSFContext *bsfc) > { > OpusBSFContext *s = bsfc->priv_data; > @@ -67,6 +62,6 @@ const AVBitStreamFilter ff_opus_metadata_bsf = { > .priv_data_size = sizeof(OpusBSFContext), > .priv_class = &opus_metadata_class, > .init = &opus_metadata_init, > - .filter = &opus_metadata_filter, > + .filter = &ff_bsf_get_packet_ref, > .codec_ids = codec_ids, > }; LGTM otherwise.
James Almer: > On 5/8/2020 7:52 AM, Andreas Rheinhardt wrote: >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> The change to the documentation would be unnecessary if James' patch >> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260984.html were >> applied. But it is only internal documentation, so changing and >> reverting it is easy. > > How does my patch change the success return values of > ff_bsf_get_packet_ref()? > It doesn't; yet av_bsf_receive_packet() documents that it returns 0 on success. Values > 0 are against its documentation. Had ff_bsf_get_packet_ref() ever returned values > 0, then forwarding its return value without filtering it (as is done currently) would not be allowed. Therefore this patch aligns the documentation of ff_bsf_get_packet_ref() with the one from av_bsf_receive_packet(). Your patch meanwhile would filter any return values > 0 of an AVBitStreamFilter.filter function away. >> >> libavcodec/bsf.h | 2 +- >> libavcodec/null_bsf.c | 7 +------ >> libavcodec/opus_metadata_bsf.c | 7 +------ >> 3 files changed, 3 insertions(+), 13 deletions(-) >> >> diff --git a/libavcodec/bsf.h b/libavcodec/bsf.h >> index af035eee44..d04f1d3068 100644 >> --- a/libavcodec/bsf.h >> +++ b/libavcodec/bsf.h >> @@ -35,7 +35,7 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt); >> * @param ctx pointer to AVBSFContext of filter >> * @param pkt pointer to packet to move reference to >> * >> - * @return 0>= on success, negative AVERROR in case of failure >> + * @return 0 on success, negative AVERROR in case of failure > > This change is ok, but I don't think it's relevant to this patch. > >> */ >> int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt); >> >> diff --git a/libavcodec/null_bsf.c b/libavcodec/null_bsf.c >> index 24d26dfb1a..37f4640c87 100644 >> --- a/libavcodec/null_bsf.c >> +++ b/libavcodec/null_bsf.c >> @@ -24,12 +24,7 @@ >> #include "avcodec.h" >> #include "bsf.h" >> >> -static int null_filter(AVBSFContext *ctx, AVPacket *pkt) >> -{ >> - return ff_bsf_get_packet_ref(ctx, pkt); >> -} >> - >> const AVBitStreamFilter ff_null_bsf = { >> .name = "null", >> - .filter = null_filter, >> + .filter = ff_bsf_get_packet_ref, >> }; >> diff --git a/libavcodec/opus_metadata_bsf.c b/libavcodec/opus_metadata_bsf.c >> index 867ad830d3..d22db54f30 100644 >> --- a/libavcodec/opus_metadata_bsf.c >> +++ b/libavcodec/opus_metadata_bsf.c >> @@ -25,11 +25,6 @@ typedef struct OpusBSFContext { >> int gain; >> } OpusBSFContext; >> >> -static int opus_metadata_filter(AVBSFContext *bsfc, AVPacket *pkt) >> -{ >> - return ff_bsf_get_packet_ref(bsfc, pkt); >> -} >> - >> static int opus_metadata_init(AVBSFContext *bsfc) >> { >> OpusBSFContext *s = bsfc->priv_data; >> @@ -67,6 +62,6 @@ const AVBitStreamFilter ff_opus_metadata_bsf = { >> .priv_data_size = sizeof(OpusBSFContext), >> .priv_class = &opus_metadata_class, >> .init = &opus_metadata_init, >> - .filter = &opus_metadata_filter, >> + .filter = &ff_bsf_get_packet_ref, >> .codec_ids = codec_ids, >> }; > > LGTM otherwise. > _______________________________________________ > 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". >
On 5/8/2020 2:37 PM, Andreas Rheinhardt wrote: > James Almer: >> On 5/8/2020 7:52 AM, Andreas Rheinhardt wrote: >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> The change to the documentation would be unnecessary if James' patch >>> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260984.html were >>> applied. But it is only internal documentation, so changing and >>> reverting it is easy. >> >> How does my patch change the success return values of >> ff_bsf_get_packet_ref()? >> > It doesn't; yet av_bsf_receive_packet() documents that it returns 0 on > success. Values > 0 are against its documentation. Had > ff_bsf_get_packet_ref() ever returned values > 0, then forwarding its > return value without filtering it (as is done currently) would not be > allowed. Therefore this patch aligns the documentation of > ff_bsf_get_packet_ref() with the one from av_bsf_receive_packet(). > Your patch meanwhile would filter any return values > 0 of an > AVBitStreamFilter.filter function away. Even without my patch ff_bsf_get_packet_ref() has no need to return > 0 for any reason, so the change is ok either way (The doxy could also mention EAGAIN and EOF are not failure, btw). Just ensure it's in a separate commit than the bsf changes when you push. > >>> >>> libavcodec/bsf.h | 2 +- >>> libavcodec/null_bsf.c | 7 +------ >>> libavcodec/opus_metadata_bsf.c | 7 +------ >>> 3 files changed, 3 insertions(+), 13 deletions(-) >>> >>> diff --git a/libavcodec/bsf.h b/libavcodec/bsf.h >>> index af035eee44..d04f1d3068 100644 >>> --- a/libavcodec/bsf.h >>> +++ b/libavcodec/bsf.h >>> @@ -35,7 +35,7 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt); >>> * @param ctx pointer to AVBSFContext of filter >>> * @param pkt pointer to packet to move reference to >>> * >>> - * @return 0>= on success, negative AVERROR in case of failure >>> + * @return 0 on success, negative AVERROR in case of failure >> >> This change is ok, but I don't think it's relevant to this patch. >> >>> */ >>> int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt); >>> >>> diff --git a/libavcodec/null_bsf.c b/libavcodec/null_bsf.c >>> index 24d26dfb1a..37f4640c87 100644 >>> --- a/libavcodec/null_bsf.c >>> +++ b/libavcodec/null_bsf.c >>> @@ -24,12 +24,7 @@ >>> #include "avcodec.h" >>> #include "bsf.h" >>> >>> -static int null_filter(AVBSFContext *ctx, AVPacket *pkt) >>> -{ >>> - return ff_bsf_get_packet_ref(ctx, pkt); >>> -} >>> - >>> const AVBitStreamFilter ff_null_bsf = { >>> .name = "null", >>> - .filter = null_filter, >>> + .filter = ff_bsf_get_packet_ref, >>> }; >>> diff --git a/libavcodec/opus_metadata_bsf.c b/libavcodec/opus_metadata_bsf.c >>> index 867ad830d3..d22db54f30 100644 >>> --- a/libavcodec/opus_metadata_bsf.c >>> +++ b/libavcodec/opus_metadata_bsf.c >>> @@ -25,11 +25,6 @@ typedef struct OpusBSFContext { >>> int gain; >>> } OpusBSFContext; >>> >>> -static int opus_metadata_filter(AVBSFContext *bsfc, AVPacket *pkt) >>> -{ >>> - return ff_bsf_get_packet_ref(bsfc, pkt); >>> -} >>> - >>> static int opus_metadata_init(AVBSFContext *bsfc) >>> { >>> OpusBSFContext *s = bsfc->priv_data; >>> @@ -67,6 +62,6 @@ const AVBitStreamFilter ff_opus_metadata_bsf = { >>> .priv_data_size = sizeof(OpusBSFContext), >>> .priv_class = &opus_metadata_class, >>> .init = &opus_metadata_init, >>> - .filter = &opus_metadata_filter, >>> + .filter = &ff_bsf_get_packet_ref, >>> .codec_ids = codec_ids, >>> }; >> >> LGTM otherwise. >> _______________________________________________ >> 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". >
diff --git a/libavcodec/bsf.h b/libavcodec/bsf.h index af035eee44..d04f1d3068 100644 --- a/libavcodec/bsf.h +++ b/libavcodec/bsf.h @@ -35,7 +35,7 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt); * @param ctx pointer to AVBSFContext of filter * @param pkt pointer to packet to move reference to * - * @return 0>= on success, negative AVERROR in case of failure + * @return 0 on success, negative AVERROR in case of failure */ int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt); diff --git a/libavcodec/null_bsf.c b/libavcodec/null_bsf.c index 24d26dfb1a..37f4640c87 100644 --- a/libavcodec/null_bsf.c +++ b/libavcodec/null_bsf.c @@ -24,12 +24,7 @@ #include "avcodec.h" #include "bsf.h" -static int null_filter(AVBSFContext *ctx, AVPacket *pkt) -{ - return ff_bsf_get_packet_ref(ctx, pkt); -} - const AVBitStreamFilter ff_null_bsf = { .name = "null", - .filter = null_filter, + .filter = ff_bsf_get_packet_ref, }; diff --git a/libavcodec/opus_metadata_bsf.c b/libavcodec/opus_metadata_bsf.c index 867ad830d3..d22db54f30 100644 --- a/libavcodec/opus_metadata_bsf.c +++ b/libavcodec/opus_metadata_bsf.c @@ -25,11 +25,6 @@ typedef struct OpusBSFContext { int gain; } OpusBSFContext; -static int opus_metadata_filter(AVBSFContext *bsfc, AVPacket *pkt) -{ - return ff_bsf_get_packet_ref(bsfc, pkt); -} - static int opus_metadata_init(AVBSFContext *bsfc) { OpusBSFContext *s = bsfc->priv_data; @@ -67,6 +62,6 @@ const AVBitStreamFilter ff_opus_metadata_bsf = { .priv_data_size = sizeof(OpusBSFContext), .priv_class = &opus_metadata_class, .init = &opus_metadata_init, - .filter = &opus_metadata_filter, + .filter = &ff_bsf_get_packet_ref, .codec_ids = codec_ids, };
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- The change to the documentation would be unnecessary if James' patch https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260984.html were applied. But it is only internal documentation, so changing and reverting it is easy. libavcodec/bsf.h | 2 +- libavcodec/null_bsf.c | 7 +------ libavcodec/opus_metadata_bsf.c | 7 +------ 3 files changed, 3 insertions(+), 13 deletions(-)