diff mbox

[FFmpeg-devel,6/8] avcodec/decode: Do not overwrite AVFrame.pkt_pos if its already set

Message ID 20190812191708.22608-6-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Aug. 12, 2019, 7:17 p.m. UTC 
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/decode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Vittorio Giovara Aug. 12, 2019, 7:21 p.m. UTC | #1 
On Mon, Aug 12, 2019 at 9:19 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/decode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 6c31166ec2..09a509d659 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -435,7 +435,7 @@ static int decode_simple_internal(AVCodecContext
> *avctx, AVFrame *frame)
>          if (!(avctx->codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS))
>              frame->pkt_dts = pkt->dts;
>          if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
> -            if(!avctx->has_b_frames)
> +            if(!avctx->has_b_frames && frame->pkt_pos < 0)
>                  frame->pkt_pos = pkt->pos;
>              //FIXME these should be under if(!avctx->has_b_frames)
>              /* get_buffer is supposed to set frame parameters */
> --
>

sure but why?
Michael Niedermayer Aug. 12, 2019, 8:31 p.m. UTC | #2 
On Mon, Aug 12, 2019 at 09:21:59PM +0200, Vittorio Giovara wrote:
> On Mon, Aug 12, 2019 at 9:19 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/decode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index 6c31166ec2..09a509d659 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -435,7 +435,7 @@ static int decode_simple_internal(AVCodecContext
> > *avctx, AVFrame *frame)
> >          if (!(avctx->codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS))
> >              frame->pkt_dts = pkt->dts;
> >          if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
> > -            if(!avctx->has_b_frames)
> > +            if(!avctx->has_b_frames && frame->pkt_pos < 0)
> >                  frame->pkt_pos = pkt->pos;
> >              //FIXME these should be under if(!avctx->has_b_frames)
> >              /* get_buffer is supposed to set frame parameters */
> > --
> >
> 
> sure but why?

the next 2 patches need this
[FFmpeg-devel] [PATCH 7/8] avcodec/qtrle: return last frame even if unchanged
[FFmpeg-devel] [PATCH 8/8] avcodec/nuv: Avoid duplicating frames

without it the common code overwrites the pkt_pos set by the decoder

thx

[...]
James Almer Aug. 13, 2019, 12:03 a.m. UTC | #3 
On 8/12/2019 5:31 PM, Michael Niedermayer wrote:
> On Mon, Aug 12, 2019 at 09:21:59PM +0200, Vittorio Giovara wrote:
>> On Mon, Aug 12, 2019 at 9:19 PM Michael Niedermayer <michael@niedermayer.cc>
>> wrote:
>>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/decode.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>> index 6c31166ec2..09a509d659 100644
>>> --- a/libavcodec/decode.c
>>> +++ b/libavcodec/decode.c
>>> @@ -435,7 +435,7 @@ static int decode_simple_internal(AVCodecContext
>>> *avctx, AVFrame *frame)
>>>          if (!(avctx->codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS))
>>>              frame->pkt_dts = pkt->dts;
>>>          if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
>>> -            if(!avctx->has_b_frames)
>>> +            if(!avctx->has_b_frames && frame->pkt_pos < 0)
>>>                  frame->pkt_pos = pkt->pos;
>>>              //FIXME these should be under if(!avctx->has_b_frames)
>>>              /* get_buffer is supposed to set frame parameters */
>>> --
>>>
>>
>> sure but why?
> 
> the next 2 patches need this
> [FFmpeg-devel] [PATCH 7/8] avcodec/qtrle: return last frame even if unchanged
> [FFmpeg-devel] [PATCH 8/8] avcodec/nuv: Avoid duplicating frames
> 
> without it the common code overwrites the pkt_pos set by the decoder

What about rawdec.c? It's also setting frame->pkt_pos.

And cant' this be handled with a new internal codec cap like the pkt_dts
one used above?

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer Aug. 14, 2019, 5:39 p.m. UTC | #4 
On Mon, Aug 12, 2019 at 09:03:22PM -0300, James Almer wrote:
> On 8/12/2019 5:31 PM, Michael Niedermayer wrote:
> > On Mon, Aug 12, 2019 at 09:21:59PM +0200, Vittorio Giovara wrote:
> >> On Mon, Aug 12, 2019 at 9:19 PM Michael Niedermayer <michael@niedermayer.cc>
> >> wrote:
> >>
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/decode.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> >>> index 6c31166ec2..09a509d659 100644
> >>> --- a/libavcodec/decode.c
> >>> +++ b/libavcodec/decode.c
> >>> @@ -435,7 +435,7 @@ static int decode_simple_internal(AVCodecContext
> >>> *avctx, AVFrame *frame)
> >>>          if (!(avctx->codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS))
> >>>              frame->pkt_dts = pkt->dts;
> >>>          if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
> >>> -            if(!avctx->has_b_frames)
> >>> +            if(!avctx->has_b_frames && frame->pkt_pos < 0)
> >>>                  frame->pkt_pos = pkt->pos;
> >>>              //FIXME these should be under if(!avctx->has_b_frames)
> >>>              /* get_buffer is supposed to set frame parameters */
> >>> --
> >>>
> >>
> >> sure but why?
> > 
> > the next 2 patches need this
> > [FFmpeg-devel] [PATCH 7/8] avcodec/qtrle: return last frame even if unchanged
> > [FFmpeg-devel] [PATCH 8/8] avcodec/nuv: Avoid duplicating frames
> > 
> > without it the common code overwrites the pkt_pos set by the decoder
> 
> What about rawdec.c? It's also setting frame->pkt_pos.

That looks redundant (will send patch after testing)


> 
> And cant' this be handled with a new internal codec cap like the pkt_dts
> one used above?

yes, if thats preferred ?

Thanks

[...]
James Almer Aug. 15, 2019, 1:46 a.m. UTC | #5 
On 8/14/2019 2:39 PM, Michael Niedermayer wrote:
> On Mon, Aug 12, 2019 at 09:03:22PM -0300, James Almer wrote:
>> On 8/12/2019 5:31 PM, Michael Niedermayer wrote:
>>> On Mon, Aug 12, 2019 at 09:21:59PM +0200, Vittorio Giovara wrote:
>>>> On Mon, Aug 12, 2019 at 9:19 PM Michael Niedermayer <michael@niedermayer.cc>
>>>> wrote:
>>>>
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>  libavcodec/decode.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>>>> index 6c31166ec2..09a509d659 100644
>>>>> --- a/libavcodec/decode.c
>>>>> +++ b/libavcodec/decode.c
>>>>> @@ -435,7 +435,7 @@ static int decode_simple_internal(AVCodecContext
>>>>> *avctx, AVFrame *frame)
>>>>>          if (!(avctx->codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS))
>>>>>              frame->pkt_dts = pkt->dts;
>>>>>          if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
>>>>> -            if(!avctx->has_b_frames)
>>>>> +            if(!avctx->has_b_frames && frame->pkt_pos < 0)
>>>>>                  frame->pkt_pos = pkt->pos;
>>>>>              //FIXME these should be under if(!avctx->has_b_frames)
>>>>>              /* get_buffer is supposed to set frame parameters */
>>>>> --
>>>>>
>>>>
>>>> sure but why?
>>>
>>> the next 2 patches need this
>>> [FFmpeg-devel] [PATCH 7/8] avcodec/qtrle: return last frame even if unchanged
>>> [FFmpeg-devel] [PATCH 8/8] avcodec/nuv: Avoid duplicating frames
>>>
>>> without it the common code overwrites the pkt_pos set by the decoder
>>
>> What about rawdec.c? It's also setting frame->pkt_pos.
> 
> That looks redundant (will send patch after testing)
> 
> 
>>
>> And cant' this be handled with a new internal codec cap like the pkt_dts
>> one used above?
> 
> yes, if thats preferred ?

I think it would be better, but not sure if others agree.

A well defined way for decoders/encoders to inform the generic code what
to expect is imo nicer than it assuming that a given value in a frame
means that it was not touched, even if said value is the default.
Perhaps it could be simplified into a single codec cap to sell all pkt_*
fields and other props (like size and duration), rather than one cap per
field. And since it's all internal, it can be removed as easily and
quickly as it was added.

> 
> Thanks
> 
> [...]
> 
> 
> _______________________________________________
> 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

Patch

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 6c31166ec2..09a509d659 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -435,7 +435,7 @@  static int decode_simple_internal(AVCodecContext *avctx, AVFrame *frame)
         if (!(avctx->codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS))
             frame->pkt_dts = pkt->dts;
         if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
-            if(!avctx->has_b_frames)
+            if(!avctx->has_b_frames && frame->pkt_pos < 0)
                 frame->pkt_pos = pkt->pos;
             //FIXME these should be under if(!avctx->has_b_frames)
             /* get_buffer is supposed to set frame parameters */