diff mbox series

[FFmpeg-devel] avcodec/(null|opus)_bsf: Use ff_bsf_get_packet_ref() directly

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt May 8, 2020, 10:52 a.m. UTC
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(-)

Comments

James Almer May 8, 2020, 1:25 p.m. UTC | #1
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.
Andreas Rheinhardt May 8, 2020, 5:37 p.m. UTC | #2
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".
>
James Almer May 8, 2020, 5:48 p.m. UTC | #3
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 mbox series

Patch

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,
 };