Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Thu, Oct 6, 2016 at 4:08 PM, Hendrik Leppkes  wrote:
> On Tue, Oct 4, 2016 at 4:44 PM, James Almer  wrote:
>> On 10/4/2016 11:35 AM, Hendrik Leppkes wrote:
>>> On Tue, Oct 4, 2016 at 4:32 PM, wm4  wrote:
 On Tue, 4 Oct 2016 14:15:03 +0200
 Michael Niedermayer  wrote:

> On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:
>> On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  
>> wrote:
>>> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
>>>  wrote:
 On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>  wrote:
>> On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>>> Decoders have previously not used AVFrame.pts, and with the upcoming
>>> deprecation of pkt_pts (in favor of pts), this would lead to an 
>>> errorneous
>>> interpration of timestamps.
>>
>> I probably misunderstand the commit message but
>> If code is changed in a user application that cannot really lift
>> some blockage from changing a lib.
>> a lib can only change in an incompaible way with (deprecation and)
>> major version bump.
>>
>
> The lib never did what this code suggests it did, not that I remember
> (so at least not for a long long time).

 release/2.0 with

 diff --git a/libavcodec/utils.c b/libavcodec/utils.c
 index 29d5492..57c8d50 100644
 --- a/libavcodec/utils.c
 +++ b/libavcodec/utils.c
 @@ -2008,7 +2008,7 @@ int attribute_align_arg 
 avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
  avci->to_free.extended_data = avci->to_free.data;
  memset(picture->buf, 0, sizeof(picture->buf));
  }
 -
 +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
  avctx->frame_number++;
  av_frame_set_best_effort_timestamp(picture,
 
 guess_correct_pts(avctx,

 causes many tests to fail, indicating that AVFrame.pts was set for
 several video decoders, the ffmpeg code is audio decoder specific
 and i failed to find a case where it was triggered, i tried IIRC 3
 or so checkouts from the past

 so AVFrame.pts was maybe never set for decoding audio but it was set
 for video
>>>
>>> Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
>>> Because thats what it would be set to after the merge. A quick check
>>> in the 2.0 code base looks like some decoders did set that, but to the
>>> exact same value as pkt_pts (which is what the merge is proposing
>>> right now as well)
>>>
>>
>> And I found this (after 2.0):
>> http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8
>>
>> Which apparently set pts for mpeg4 to a number parsed from the
>> bitstream, entirely uncorrelated to container or audio timestamps, so
>> using them would have been rather impractical for any real use-cases.
>
> They can be usfull, some random examples:
>
> playing MPEG4-ES with timing stored from the bitstream (assuming there
>   is no demuxer lib used that is capable to extract them)
>
> forensics, raw video stream could have its timing
>   recovered, a video with manipulated container timestamps could be
>   detected.
>
> error correction, if the container level timestamps are lost or
>   corrupted the stream level ones can be used to recreate them
>
> There may be more, these are just some of the top of my head,
> not your mainstream multimedia player stuff maybe but they arent
> useless
>
> [...]
>

 They don't belong into the AVFrame.pts field, though.
>>>
>>> And they don't go in there anymore right now, so thats that.
>>>
>>> The real question is, what do we do about this merge now?
>>> Can we set AVFrame.pts to the same value as AVFrame.pkt_pts safely,
>>> considering it was unused in the current ABI/API, or would that be
>>> considered an API break and we better delay this change until the next
>>> major?
>>>
>>> - Hendrik
>>
>> Delaying it could result in further merges becoming technically wrong,
>> or at least require extra manual changes for them to not misbehave in
>> our tree.
>>
>> IMO merge it now, and if needed/preferred, we could make sure it
>> doesn't make it to 3.2
>>
>
> Last call for any actual and clear objections to going forward with
> this route. I would like to get merging a bunch over the weekend 

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Tue, Oct 4, 2016 at 4:44 PM, James Almer  wrote:
> On 10/4/2016 11:35 AM, Hendrik Leppkes wrote:
>> On Tue, Oct 4, 2016 at 4:32 PM, wm4  wrote:
>>> On Tue, 4 Oct 2016 14:15:03 +0200
>>> Michael Niedermayer  wrote:
>>>
 On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:
> On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  
> wrote:
>> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
>>  wrote:
>>> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
 On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
  wrote:
> On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>> Decoders have previously not used AVFrame.pts, and with the upcoming
>> deprecation of pkt_pts (in favor of pts), this would lead to an 
>> errorneous
>> interpration of timestamps.
>
> I probably misunderstand the commit message but
> If code is changed in a user application that cannot really lift
> some blockage from changing a lib.
> a lib can only change in an incompaible way with (deprecation and)
> major version bump.
>

 The lib never did what this code suggests it did, not that I remember
 (so at least not for a long long time).
>>>
>>> release/2.0 with
>>>
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index 29d5492..57c8d50 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -2008,7 +2008,7 @@ int attribute_align_arg 
>>> avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
>>>  avci->to_free.extended_data = avci->to_free.data;
>>>  memset(picture->buf, 0, sizeof(picture->buf));
>>>  }
>>> -
>>> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
>>>  avctx->frame_number++;
>>>  av_frame_set_best_effort_timestamp(picture,
>>> guess_correct_pts(avctx,
>>>
>>> causes many tests to fail, indicating that AVFrame.pts was set for
>>> several video decoders, the ffmpeg code is audio decoder specific
>>> and i failed to find a case where it was triggered, i tried IIRC 3
>>> or so checkouts from the past
>>>
>>> so AVFrame.pts was maybe never set for decoding audio but it was set
>>> for video
>>
>> Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
>> Because thats what it would be set to after the merge. A quick check
>> in the 2.0 code base looks like some decoders did set that, but to the
>> exact same value as pkt_pts (which is what the merge is proposing
>> right now as well)
>>
>
> And I found this (after 2.0):
> http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8
>
> Which apparently set pts for mpeg4 to a number parsed from the
> bitstream, entirely uncorrelated to container or audio timestamps, so
> using them would have been rather impractical for any real use-cases.

 They can be usfull, some random examples:

 playing MPEG4-ES with timing stored from the bitstream (assuming there
   is no demuxer lib used that is capable to extract them)

 forensics, raw video stream could have its timing
   recovered, a video with manipulated container timestamps could be
   detected.

 error correction, if the container level timestamps are lost or
   corrupted the stream level ones can be used to recreate them

 There may be more, these are just some of the top of my head,
 not your mainstream multimedia player stuff maybe but they arent
 useless

 [...]

>>>
>>> They don't belong into the AVFrame.pts field, though.
>>
>> And they don't go in there anymore right now, so thats that.
>>
>> The real question is, what do we do about this merge now?
>> Can we set AVFrame.pts to the same value as AVFrame.pkt_pts safely,
>> considering it was unused in the current ABI/API, or would that be
>> considered an API break and we better delay this change until the next
>> major?
>>
>> - Hendrik
>
> Delaying it could result in further merges becoming technically wrong,
> or at least require extra manual changes for them to not misbehave in
> our tree.
>
> IMO merge it now, and if needed/preferred, we could make sure it
> doesn't make it to 3.2
>

Last call for any actual and clear objections to going forward with
this route. I would like to get merging a bunch over the weekend so we
get some progress here.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[James Almer]

On 10/4/2016 11:35 AM, Hendrik Leppkes wrote:
> On Tue, Oct 4, 2016 at 4:32 PM, wm4  wrote:
>> On Tue, 4 Oct 2016 14:15:03 +0200
>> Michael Niedermayer  wrote:
>>
>>> On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:
 On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  
 wrote:
> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
>  wrote:
>> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
>>> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>>>  wrote:
 On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
> Decoders have previously not used AVFrame.pts, and with the upcoming
> deprecation of pkt_pts (in favor of pts), this would lead to an 
> errorneous
> interpration of timestamps.

 I probably misunderstand the commit message but
 If code is changed in a user application that cannot really lift
 some blockage from changing a lib.
 a lib can only change in an incompaible way with (deprecation and)
 major version bump.

>>>
>>> The lib never did what this code suggests it did, not that I remember
>>> (so at least not for a long long time).
>>
>> release/2.0 with
>>
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index 29d5492..57c8d50 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -2008,7 +2008,7 @@ int attribute_align_arg 
>> avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
>>  avci->to_free.extended_data = avci->to_free.data;
>>  memset(picture->buf, 0, sizeof(picture->buf));
>>  }
>> -
>> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
>>  avctx->frame_number++;
>>  av_frame_set_best_effort_timestamp(picture,
>> guess_correct_pts(avctx,
>>
>> causes many tests to fail, indicating that AVFrame.pts was set for
>> several video decoders, the ffmpeg code is audio decoder specific
>> and i failed to find a case where it was triggered, i tried IIRC 3
>> or so checkouts from the past
>>
>> so AVFrame.pts was maybe never set for decoding audio but it was set
>> for video
>
> Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
> Because thats what it would be set to after the merge. A quick check
> in the 2.0 code base looks like some decoders did set that, but to the
> exact same value as pkt_pts (which is what the merge is proposing
> right now as well)
>

 And I found this (after 2.0):
 http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8

 Which apparently set pts for mpeg4 to a number parsed from the
 bitstream, entirely uncorrelated to container or audio timestamps, so
 using them would have been rather impractical for any real use-cases.
>>>
>>> They can be usfull, some random examples:
>>>
>>> playing MPEG4-ES with timing stored from the bitstream (assuming there
>>>   is no demuxer lib used that is capable to extract them)
>>>
>>> forensics, raw video stream could have its timing
>>>   recovered, a video with manipulated container timestamps could be
>>>   detected.
>>>
>>> error correction, if the container level timestamps are lost or
>>>   corrupted the stream level ones can be used to recreate them
>>>
>>> There may be more, these are just some of the top of my head,
>>> not your mainstream multimedia player stuff maybe but they arent
>>> useless
>>>
>>> [...]
>>>
>>
>> They don't belong into the AVFrame.pts field, though.
> 
> And they don't go in there anymore right now, so thats that.
> 
> The real question is, what do we do about this merge now?
> Can we set AVFrame.pts to the same value as AVFrame.pkt_pts safely,
> considering it was unused in the current ABI/API, or would that be
> considered an API break and we better delay this change until the next
> major?
> 
> - Hendrik

Delaying it could result in further merges becoming technically wrong,
or at least require extra manual changes for them to not misbehave in
our tree.

IMO merge it now, and if needed/preferred, we could make sure it
doesn't make it to 3.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[wm4]

On Tue, 4 Oct 2016 16:35:02 +0200
Hendrik Leppkes  wrote:

> On Tue, Oct 4, 2016 at 4:32 PM, wm4  wrote:
> > On Tue, 4 Oct 2016 14:15:03 +0200
> > Michael Niedermayer  wrote:
> >  
> >> On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:  
> >> > On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  
> >> > wrote:  
> >> > > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
> >> > >  wrote:  
> >> > >> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:  
> >> > >>> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
> >> > >>>  wrote:  
> >> > >>> > On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:  
> >> > >>> >> Decoders have previously not used AVFrame.pts, and with the 
> >> > >>> >> upcoming
> >> > >>> >> deprecation of pkt_pts (in favor of pts), this would lead to an 
> >> > >>> >> errorneous
> >> > >>> >> interpration of timestamps.  
> >> > >>> >
> >> > >>> > I probably misunderstand the commit message but
> >> > >>> > If code is changed in a user application that cannot really lift
> >> > >>> > some blockage from changing a lib.
> >> > >>> > a lib can only change in an incompaible way with (deprecation and)
> >> > >>> > major version bump.
> >> > >>> >  
> >> > >>>
> >> > >>> The lib never did what this code suggests it did, not that I remember
> >> > >>> (so at least not for a long long time).  
> >> > >>
> >> > >> release/2.0 with
> >> > >>
> >> > >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> >> > >> index 29d5492..57c8d50 100644
> >> > >> --- a/libavcodec/utils.c
> >> > >> +++ b/libavcodec/utils.c
> >> > >> @@ -2008,7 +2008,7 @@ int attribute_align_arg 
> >> > >> avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
> >> > >>  avci->to_free.extended_data = avci->to_free.data;
> >> > >>  memset(picture->buf, 0, sizeof(picture->buf));
> >> > >>  }
> >> > >> -
> >> > >> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
> >> > >>  avctx->frame_number++;
> >> > >>  av_frame_set_best_effort_timestamp(picture,
> >> > >> 
> >> > >> guess_correct_pts(avctx,
> >> > >>
> >> > >> causes many tests to fail, indicating that AVFrame.pts was set for
> >> > >> several video decoders, the ffmpeg code is audio decoder specific
> >> > >> and i failed to find a case where it was triggered, i tried IIRC 3
> >> > >> or so checkouts from the past
> >> > >>
> >> > >> so AVFrame.pts was maybe never set for decoding audio but it was set
> >> > >> for video  
> >> > >
> >> > > Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
> >> > > Because thats what it would be set to after the merge. A quick check
> >> > > in the 2.0 code base looks like some decoders did set that, but to the
> >> > > exact same value as pkt_pts (which is what the merge is proposing
> >> > > right now as well)
> >> > >  
> >> >
> >> > And I found this (after 2.0):
> >> > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8
> >> >
> >> > Which apparently set pts for mpeg4 to a number parsed from the
> >> > bitstream, entirely uncorrelated to container or audio timestamps, so
> >> > using them would have been rather impractical for any real use-cases.  
> >>
> >> They can be usfull, some random examples:
> >>
> >> playing MPEG4-ES with timing stored from the bitstream (assuming there
> >>   is no demuxer lib used that is capable to extract them)
> >>
> >> forensics, raw video stream could have its timing
> >>   recovered, a video with manipulated container timestamps could be
> >>   detected.
> >>
> >> error correction, if the container level timestamps are lost or
> >>   corrupted the stream level ones can be used to recreate them
> >>
> >> There may be more, these are just some of the top of my head,
> >> not your mainstream multimedia player stuff maybe but they arent
> >> useless
> >>
> >> [...]
> >>  
> >
> > They don't belong into the AVFrame.pts field, though.  
> 
> And they don't go in there anymore right now, so thats that.
> 
> The real question is, what do we do about this merge now?
> Can we set AVFrame.pts to the same value as AVFrame.pkt_pts safely,
> considering it was unused in the current ABI/API, or would that be
> considered an API break and we better delay this change until the next
> major?

IMO applications which did this were pretty broken anyway, and we
should be able to get away with a simple version bump.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Tue, Oct 4, 2016 at 4:32 PM, wm4  wrote:
> On Tue, 4 Oct 2016 14:15:03 +0200
> Michael Niedermayer  wrote:
>
>> On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:
>> > On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  
>> > wrote:
>> > > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
>> > >  wrote:
>> > >> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
>> > >>> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>> > >>>  wrote:
>> > >>> > On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>> > >>> >> Decoders have previously not used AVFrame.pts, and with the upcoming
>> > >>> >> deprecation of pkt_pts (in favor of pts), this would lead to an 
>> > >>> >> errorneous
>> > >>> >> interpration of timestamps.
>> > >>> >
>> > >>> > I probably misunderstand the commit message but
>> > >>> > If code is changed in a user application that cannot really lift
>> > >>> > some blockage from changing a lib.
>> > >>> > a lib can only change in an incompaible way with (deprecation and)
>> > >>> > major version bump.
>> > >>> >
>> > >>>
>> > >>> The lib never did what this code suggests it did, not that I remember
>> > >>> (so at least not for a long long time).
>> > >>
>> > >> release/2.0 with
>> > >>
>> > >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> > >> index 29d5492..57c8d50 100644
>> > >> --- a/libavcodec/utils.c
>> > >> +++ b/libavcodec/utils.c
>> > >> @@ -2008,7 +2008,7 @@ int attribute_align_arg 
>> > >> avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
>> > >>  avci->to_free.extended_data = avci->to_free.data;
>> > >>  memset(picture->buf, 0, sizeof(picture->buf));
>> > >>  }
>> > >> -
>> > >> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
>> > >>  avctx->frame_number++;
>> > >>  av_frame_set_best_effort_timestamp(picture,
>> > >> guess_correct_pts(avctx,
>> > >>
>> > >> causes many tests to fail, indicating that AVFrame.pts was set for
>> > >> several video decoders, the ffmpeg code is audio decoder specific
>> > >> and i failed to find a case where it was triggered, i tried IIRC 3
>> > >> or so checkouts from the past
>> > >>
>> > >> so AVFrame.pts was maybe never set for decoding audio but it was set
>> > >> for video
>> > >
>> > > Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
>> > > Because thats what it would be set to after the merge. A quick check
>> > > in the 2.0 code base looks like some decoders did set that, but to the
>> > > exact same value as pkt_pts (which is what the merge is proposing
>> > > right now as well)
>> > >
>> >
>> > And I found this (after 2.0):
>> > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8
>> >
>> > Which apparently set pts for mpeg4 to a number parsed from the
>> > bitstream, entirely uncorrelated to container or audio timestamps, so
>> > using them would have been rather impractical for any real use-cases.
>>
>> They can be usfull, some random examples:
>>
>> playing MPEG4-ES with timing stored from the bitstream (assuming there
>>   is no demuxer lib used that is capable to extract them)
>>
>> forensics, raw video stream could have its timing
>>   recovered, a video with manipulated container timestamps could be
>>   detected.
>>
>> error correction, if the container level timestamps are lost or
>>   corrupted the stream level ones can be used to recreate them
>>
>> There may be more, these are just some of the top of my head,
>> not your mainstream multimedia player stuff maybe but they arent
>> useless
>>
>> [...]
>>
>
> They don't belong into the AVFrame.pts field, though.

And they don't go in there anymore right now, so thats that.

The real question is, what do we do about this merge now?
Can we set AVFrame.pts to the same value as AVFrame.pkt_pts safely,
considering it was unused in the current ABI/API, or would that be
considered an API break and we better delay this change until the next
major?

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[wm4]

On Tue, 4 Oct 2016 14:15:03 +0200
Michael Niedermayer  wrote:

> On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:
> > On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  
> > wrote:  
> > > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
> > >  wrote:  
> > >> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:  
> > >>> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
> > >>>  wrote:  
> > >>> > On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:  
> > >>> >> Decoders have previously not used AVFrame.pts, and with the upcoming
> > >>> >> deprecation of pkt_pts (in favor of pts), this would lead to an 
> > >>> >> errorneous
> > >>> >> interpration of timestamps.  
> > >>> >
> > >>> > I probably misunderstand the commit message but
> > >>> > If code is changed in a user application that cannot really lift
> > >>> > some blockage from changing a lib.
> > >>> > a lib can only change in an incompaible way with (deprecation and)
> > >>> > major version bump.
> > >>> >  
> > >>>
> > >>> The lib never did what this code suggests it did, not that I remember
> > >>> (so at least not for a long long time).  
> > >>
> > >> release/2.0 with
> > >>
> > >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > >> index 29d5492..57c8d50 100644
> > >> --- a/libavcodec/utils.c
> > >> +++ b/libavcodec/utils.c
> > >> @@ -2008,7 +2008,7 @@ int attribute_align_arg 
> > >> avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
> > >>  avci->to_free.extended_data = avci->to_free.data;
> > >>  memset(picture->buf, 0, sizeof(picture->buf));
> > >>  }
> > >> -
> > >> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
> > >>  avctx->frame_number++;
> > >>  av_frame_set_best_effort_timestamp(picture,
> > >> guess_correct_pts(avctx,
> > >>
> > >> causes many tests to fail, indicating that AVFrame.pts was set for
> > >> several video decoders, the ffmpeg code is audio decoder specific
> > >> and i failed to find a case where it was triggered, i tried IIRC 3
> > >> or so checkouts from the past
> > >>
> > >> so AVFrame.pts was maybe never set for decoding audio but it was set
> > >> for video  
> > >
> > > Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
> > > Because thats what it would be set to after the merge. A quick check
> > > in the 2.0 code base looks like some decoders did set that, but to the
> > > exact same value as pkt_pts (which is what the merge is proposing
> > > right now as well)
> > >  
> > 
> > And I found this (after 2.0):
> > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8
> > 
> > Which apparently set pts for mpeg4 to a number parsed from the
> > bitstream, entirely uncorrelated to container or audio timestamps, so
> > using them would have been rather impractical for any real use-cases.  
> 
> They can be usfull, some random examples:
> 
> playing MPEG4-ES with timing stored from the bitstream (assuming there
>   is no demuxer lib used that is capable to extract them)
> 
> forensics, raw video stream could have its timing
>   recovered, a video with manipulated container timestamps could be
>   detected.
> 
> error correction, if the container level timestamps are lost or
>   corrupted the stream level ones can be used to recreate them
> 
> There may be more, these are just some of the top of my head,
> not your mainstream multimedia player stuff maybe but they arent
> useless
> 
> [...]
> 

They don't belong into the AVFrame.pts field, though.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Michael Niedermayer]

On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:
> On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  wrote:
> > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
> >  wrote:
> >> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
> >>> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
> >>>  wrote:
> >>> > On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
> >>> >> Decoders have previously not used AVFrame.pts, and with the upcoming
> >>> >> deprecation of pkt_pts (in favor of pts), this would lead to an 
> >>> >> errorneous
> >>> >> interpration of timestamps.
> >>> >
> >>> > I probably misunderstand the commit message but
> >>> > If code is changed in a user application that cannot really lift
> >>> > some blockage from changing a lib.
> >>> > a lib can only change in an incompaible way with (deprecation and)
> >>> > major version bump.
> >>> >
> >>>
> >>> The lib never did what this code suggests it did, not that I remember
> >>> (so at least not for a long long time).
> >>
> >> release/2.0 with
> >>
> >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> >> index 29d5492..57c8d50 100644
> >> --- a/libavcodec/utils.c
> >> +++ b/libavcodec/utils.c
> >> @@ -2008,7 +2008,7 @@ int attribute_align_arg 
> >> avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
> >>  avci->to_free.extended_data = avci->to_free.data;
> >>  memset(picture->buf, 0, sizeof(picture->buf));
> >>  }
> >> -
> >> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
> >>  avctx->frame_number++;
> >>  av_frame_set_best_effort_timestamp(picture,
> >> guess_correct_pts(avctx,
> >>
> >> causes many tests to fail, indicating that AVFrame.pts was set for
> >> several video decoders, the ffmpeg code is audio decoder specific
> >> and i failed to find a case where it was triggered, i tried IIRC 3
> >> or so checkouts from the past
> >>
> >> so AVFrame.pts was maybe never set for decoding audio but it was set
> >> for video
> >
> > Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
> > Because thats what it would be set to after the merge. A quick check
> > in the 2.0 code base looks like some decoders did set that, but to the
> > exact same value as pkt_pts (which is what the merge is proposing
> > right now as well)
> >
> 
> And I found this (after 2.0):
> http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8
> 
> Which apparently set pts for mpeg4 to a number parsed from the
> bitstream, entirely uncorrelated to container or audio timestamps, so
> using them would have been rather impractical for any real use-cases.

They can be usfull, some random examples:

playing MPEG4-ES with timing stored from the bitstream (assuming there
  is no demuxer lib used that is capable to extract them)

forensics, raw video stream could have its timing
  recovered, a video with manipulated container timestamps could be
  detected.

error correction, if the container level timestamps are lost or
  corrupted the stream level ones can be used to recreate them

There may be more, these are just some of the top of my head,
not your mainstream multimedia player stuff maybe but they arent
useless

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Tue, Oct 4, 2016 at 1:55 PM, Michael Niedermayer
 wrote:
> On Tue, Oct 04, 2016 at 10:30:24AM +0200, Hendrik Leppkes wrote:
>> On Tue, Oct 4, 2016 at 8:41 AM, Hendrik Leppkes  wrote:
>> > On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>> >  wrote:
>> >> On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>> >>> Decoders have previously not used AVFrame.pts, and with the upcoming
>> >>> deprecation of pkt_pts (in favor of pts), this would lead to an 
>> >>> errorneous
>> >>> interpration of timestamps.
>> >>
>> >> I probably misunderstand the commit message but
>> >> If code is changed in a user application that cannot really lift
>> >> some blockage from changing a lib.
>> >> a lib can only change in an incompaible way with (deprecation and)
>> >> major version bump.
>> >>
>> >
>> > The lib never did what this code suggests it did, not that I remember
>> > (so at least not for a long long time).
>> >
>>
>> Of course that could still mean that some other apps "copied" the
>> ffmpeg code and try to read this field - but is this a scenario we can
>> really control?
>>
>> The pts field in AVFrame is currently unused for decoding, nothing
>> sets it (except cuvid and openh264 or so, but those set it the same
>> way it would be set in the future, so no changes there), ffmpeg.c
>> trying to read it is a remnant from a long time ago (quick blame pins
>> it at 2012 when decoding was changed to decode_audio4).
>> I couldn't actually confirm if at that time (audio) decoders did even
>> set AVFrame.pts, considering pkt_pts already existed then.
>>
>> So is starting to set a field that was previously (at least through 2
>> or so major bumps) unused a API break?
>> Its always possible some app still tries to read it, but because its
>> never set it didn't cause any problems so far.
>>
>> The alternative is of course to keep using pkt_pts, and keep pts
>> unused for decoding, but I'm not entirely convinced there is a break
>> here.
>
> not stating any oppinion in this paragraph but
> If use of AVFrame.pts is considered a bug in the current API then past
> releases with that API need to be fixed. If they are not fixed
> testing API/ABI for 3.2 will blow up (i havnt tried it yet for 3.2
> but i did for past releases previously and mixing libs between releases
> and ffmpeg is required to not blow up, it protects against API/ABI
> breaks somewhat)
> Also release notes for 3.2 would be needed as current 3.1 would not
> mix well with a release with same soname and differently used
> AVFrame.pts. That is unless i miss something
>
> Somewhat off topic and my personal oppinion
> I think independant of field names and API, there are 3 or 4 types of
> timestamps, it would be good if user applications have some easy
> way of accessing all of them for a frame from a decoder.
> the 4 types are,
> * input AVPacket.pts based
> * input AVPacket.dts based
> * what is stored in the codec bitstream if any
> * some easy to use one that simple apps can just use and not need to
>   worry about anything, aka one that is "correct" in almost all cases
>

Well we have 3 of those, and nothing that provides the codec
timestamps (not sure they would even exist anywhere).
If the 4th kind is added, it should definitely not be in AVFrame.pts
however, since people might mistakenly use that as a general purpose
timestamp.

The entire reason for the upcoming change is simple:
- AVFrame.pts is used for the timestamp in filtering and encoding, but
unused in decoding, which is inconsistent
- AVFrame.pkt_pts is only used for decoding, unused anywhere else.

So combining those into one field seems like a logical step.
AVFrame.pts has been unused for decoding for ~3 years (including
several major bumps), and even before that its use was probably not
very useful.

Is that enough time to recycle it? Or should we rather skip the change
and possibly queue it for later (ie. the next bump)?

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Michael Niedermayer]

On Tue, Oct 04, 2016 at 10:30:24AM +0200, Hendrik Leppkes wrote:
> On Tue, Oct 4, 2016 at 8:41 AM, Hendrik Leppkes  wrote:
> > On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
> >  wrote:
> >> On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
> >>> Decoders have previously not used AVFrame.pts, and with the upcoming
> >>> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
> >>> interpration of timestamps.
> >>
> >> I probably misunderstand the commit message but
> >> If code is changed in a user application that cannot really lift
> >> some blockage from changing a lib.
> >> a lib can only change in an incompaible way with (deprecation and)
> >> major version bump.
> >>
> >
> > The lib never did what this code suggests it did, not that I remember
> > (so at least not for a long long time).
> >
> 
> Of course that could still mean that some other apps "copied" the
> ffmpeg code and try to read this field - but is this a scenario we can
> really control?
> 
> The pts field in AVFrame is currently unused for decoding, nothing
> sets it (except cuvid and openh264 or so, but those set it the same
> way it would be set in the future, so no changes there), ffmpeg.c
> trying to read it is a remnant from a long time ago (quick blame pins
> it at 2012 when decoding was changed to decode_audio4).
> I couldn't actually confirm if at that time (audio) decoders did even
> set AVFrame.pts, considering pkt_pts already existed then.
> 
> So is starting to set a field that was previously (at least through 2
> or so major bumps) unused a API break?
> Its always possible some app still tries to read it, but because its
> never set it didn't cause any problems so far.
> 
> The alternative is of course to keep using pkt_pts, and keep pts
> unused for decoding, but I'm not entirely convinced there is a break
> here.

not stating any oppinion in this paragraph but
If use of AVFrame.pts is considered a bug in the current API then past
releases with that API need to be fixed. If they are not fixed
testing API/ABI for 3.2 will blow up (i havnt tried it yet for 3.2
but i did for past releases previously and mixing libs between releases
and ffmpeg is required to not blow up, it protects against API/ABI
breaks somewhat)
Also release notes for 3.2 would be needed as current 3.1 would not
mix well with a release with same soname and differently used
AVFrame.pts. That is unless i miss something

Somewhat off topic and my personal oppinion
I think independant of field names and API, there are 3 or 4 types of
timestamps, it would be good if user applications have some easy
way of accessing all of them for a frame from a decoder.
the 4 types are,
* input AVPacket.pts based
* input AVPacket.dts based
* what is stored in the codec bitstream if any
* some easy to use one that simple apps can just use and not need to
  worry about anything, aka one that is "correct" in almost all cases


And of course i want the next release to work for distros and not
see complaints about it causing pain due to API/ABI stuff, breaking
packages and so on.

And i want a clean, easy to use, maintainable and yet powerfull API

About the original patch in this thread itself i have no real oppinon

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  wrote:
> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
>  wrote:
>> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
>>> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>>>  wrote:
>>> > On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>>> >> Decoders have previously not used AVFrame.pts, and with the upcoming
>>> >> deprecation of pkt_pts (in favor of pts), this would lead to an 
>>> >> errorneous
>>> >> interpration of timestamps.
>>> >
>>> > I probably misunderstand the commit message but
>>> > If code is changed in a user application that cannot really lift
>>> > some blockage from changing a lib.
>>> > a lib can only change in an incompaible way with (deprecation and)
>>> > major version bump.
>>> >
>>>
>>> The lib never did what this code suggests it did, not that I remember
>>> (so at least not for a long long time).
>>
>> release/2.0 with
>>
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index 29d5492..57c8d50 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -2008,7 +2008,7 @@ int attribute_align_arg 
>> avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
>>  avci->to_free.extended_data = avci->to_free.data;
>>  memset(picture->buf, 0, sizeof(picture->buf));
>>  }
>> -
>> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
>>  avctx->frame_number++;
>>  av_frame_set_best_effort_timestamp(picture,
>> guess_correct_pts(avctx,
>>
>> causes many tests to fail, indicating that AVFrame.pts was set for
>> several video decoders, the ffmpeg code is audio decoder specific
>> and i failed to find a case where it was triggered, i tried IIRC 3
>> or so checkouts from the past
>>
>> so AVFrame.pts was maybe never set for decoding audio but it was set
>> for video
>
> Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
> Because thats what it would be set to after the merge. A quick check
> in the 2.0 code base looks like some decoders did set that, but to the
> exact same value as pkt_pts (which is what the merge is proposing
> right now as well)
>

And I found this (after 2.0):
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8

Which apparently set pts for mpeg4 to a number parsed from the
bitstream, entirely uncorrelated to container or audio timestamps, so
using them would have been rather impractical for any real use-cases.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
 wrote:
> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
>> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>>  wrote:
>> > On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>> >> Decoders have previously not used AVFrame.pts, and with the upcoming
>> >> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
>> >> interpration of timestamps.
>> >
>> > I probably misunderstand the commit message but
>> > If code is changed in a user application that cannot really lift
>> > some blockage from changing a lib.
>> > a lib can only change in an incompaible way with (deprecation and)
>> > major version bump.
>> >
>>
>> The lib never did what this code suggests it did, not that I remember
>> (so at least not for a long long time).
>
> release/2.0 with
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 29d5492..57c8d50 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -2008,7 +2008,7 @@ int attribute_align_arg 
> avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
>  avci->to_free.extended_data = avci->to_free.data;
>  memset(picture->buf, 0, sizeof(picture->buf));
>  }
> -
> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
>  avctx->frame_number++;
>  av_frame_set_best_effort_timestamp(picture,
> guess_correct_pts(avctx,
>
> causes many tests to fail, indicating that AVFrame.pts was set for
> several video decoders, the ffmpeg code is audio decoder specific
> and i failed to find a case where it was triggered, i tried IIRC 3
> or so checkouts from the past
>
> so AVFrame.pts was maybe never set for decoding audio but it was set
> for video

Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
Because thats what it would be set to after the merge. A quick check
in the 2.0 code base looks like some decoders did set that, but to the
exact same value as pkt_pts (which is what the merge is proposing
right now as well)

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Michael Niedermayer]

On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>  wrote:
> > On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
> >> Decoders have previously not used AVFrame.pts, and with the upcoming
> >> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
> >> interpration of timestamps.
> >
> > I probably misunderstand the commit message but
> > If code is changed in a user application that cannot really lift
> > some blockage from changing a lib.
> > a lib can only change in an incompaible way with (deprecation and)
> > major version bump.
> >
> 
> The lib never did what this code suggests it did, not that I remember
> (so at least not for a long long time).

release/2.0 with

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 29d5492..57c8d50 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -2008,7 +2008,7 @@ int attribute_align_arg 
avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
 avci->to_free.extended_data = avci->to_free.data;
 memset(picture->buf, 0, sizeof(picture->buf));
 }
-
+av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
 avctx->frame_number++;
 av_frame_set_best_effort_timestamp(picture,
guess_correct_pts(avctx,

causes many tests to fail, indicating that AVFrame.pts was set for
several video decoders, the ffmpeg code is audio decoder specific
and i failed to find a case where it was triggered, i tried IIRC 3
or so checkouts from the past

so AVFrame.pts was maybe never set for decoding audio but it was set
for video

make: *** [fate-force_key_frames] Error 134
make: *** [fate-vsynth1-h263] Error 134
make: *** [fate-vsynth1-h263-obmc] Error 134
make: *** [fate-vsynth1-h263p] Error 134
make: *** [fate-vsynth1-mpeg4-rc] Error 134
make: *** [fate-vsynth1-mpeg4] Error 134
make: *** [fate-vsynth1-mpeg4-error] Error 134
make: *** [fate-vsynth1-mpeg4-nr] Error 134
make: *** [fate-vsynth1-mpeg4-adv] Error 134
make: *** [fate-vsynth1-mpeg4-thread] Error 134
make: *** [fate-vsynth1-mpeg4-qpel] Error 134
make: *** [fate-vsynth1-mpeg4-adap] Error 134
make: *** [fate-vsynth1-mpeg4-qprd] Error 134
make: *** [fate-vsynth2-h263] Error 134
make: *** [fate-vsynth2-h263-obmc] Error 134
make: *** [fate-vsynth2-h263p] Error 134
make: *** [fate-vsynth2-mpeg4] Error 134
make: *** [fate-vsynth2-mpeg4-rc] Error 134
make: *** [fate-vsynth2-mpeg4-adv] Error 134
make: *** [fate-vsynth2-mpeg4-error] Error 134
make: *** [fate-vsynth2-mpeg4-thread] Error 134
make: *** [fate-vsynth2-mpeg4-nr] Error 134
make: *** [fate-vsynth2-mpeg4-adap] Error 134
make: *** [fate-vsynth2-mpeg4-qprd] Error 134
make: *** [fate-vsynth2-mpeg4-qpel] Error 134
make: *** [fate-lavf-avi] Error 134
make: *** [fate-lavf-gif] Error 134
make: *** [fate-lavf-mkv] Error 134
make: *** [fate-lavf-ismv] Error 134
make: *** [fate-lavf-mov] Error 134
make: *** [fate-lavf-nut] Error 134
make: *** [fate-gif-color] Error 134
make: *** [fate-gif-disposal-background] Error 134
make: *** [fate-gif-disposal-restore] Error 134
make: *** [fate-gif-gray] Error 134
make: *** [fate-gifenc-rgb8] Error 134
make: *** [fate-gifenc-bgr8] Error 134
make: *** [fate-gifenc-rgb4_byte] Error 134
make: *** [fate-gifenc-gray] Error 134
make: *** [fate-gifenc-bgr4_byte] Error 134
make: *** [fate-gifenc-pal8] Error 134

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Tue, Oct 4, 2016 at 8:41 AM, Hendrik Leppkes  wrote:
> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>  wrote:
>> On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>>> Decoders have previously not used AVFrame.pts, and with the upcoming
>>> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
>>> interpration of timestamps.
>>
>> I probably misunderstand the commit message but
>> If code is changed in a user application that cannot really lift
>> some blockage from changing a lib.
>> a lib can only change in an incompaible way with (deprecation and)
>> major version bump.
>>
>
> The lib never did what this code suggests it did, not that I remember
> (so at least not for a long long time).
>

Of course that could still mean that some other apps "copied" the
ffmpeg code and try to read this field - but is this a scenario we can
really control?

The pts field in AVFrame is currently unused for decoding, nothing
sets it (except cuvid and openh264 or so, but those set it the same
way it would be set in the future, so no changes there), ffmpeg.c
trying to read it is a remnant from a long time ago (quick blame pins
it at 2012 when decoding was changed to decode_audio4).
I couldn't actually confirm if at that time (audio) decoders did even
set AVFrame.pts, considering pkt_pts already existed then.

So is starting to set a field that was previously (at least through 2
or so major bumps) unused a API break?
Its always possible some app still tries to read it, but because its
never set it didn't cause any problems so far.

The alternative is of course to keep using pkt_pts, and keep pts
unused for decoding, but I'm not entirely convinced there is a break
here.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
 wrote:
> On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>> Decoders have previously not used AVFrame.pts, and with the upcoming
>> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
>> interpration of timestamps.
>
> I probably misunderstand the commit message but
> If code is changed in a user application that cannot really lift
> some blockage from changing a lib.
> a lib can only change in an incompaible way with (deprecation and)
> major version bump.
>

The lib never did what this code suggests it did, not that I remember
(so at least not for a long long time).

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Michael Niedermayer]

On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
> Decoders have previously not used AVFrame.pts, and with the upcoming
> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
> interpration of timestamps.

I probably misunderstand the commit message but
If code is changed in a user application that cannot really lift
some blockage from changing a lib.
a lib can only change in an incompaible way with (deprecation and)
major version bump.

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Sun, Oct 2, 2016 at 6:22 PM, Hendrik Leppkes  wrote:
> On Sat, Oct 1, 2016 at 4:15 PM, Hendrik Leppkes  wrote:
>> Decoders have previously not used AVFrame.pts, and with the upcoming
>> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
>> interpration of timestamps.
>> ---
>>  ffmpeg.c | 7 +--
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index 9a8e65a..cdbf3d4 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -2058,12 +2058,7 @@ static int decode_audio(InputStream *ist, AVPacket 
>> *pkt, int *got_output)
>>  }
>>  }
>>
>> -/* if the decoder provides a pts, use it instead of the last packet pts.
>> -   the decoder could be delaying output by a packet or more. */
>> -if (decoded_frame->pts != AV_NOPTS_VALUE) {
>> -ist->dts = ist->next_dts = ist->pts = ist->next_pts = 
>> av_rescale_q(decoded_frame->pts, avctx->time_base, AV_TIME_BASE_Q);
>> -decoded_frame_tb   = avctx->time_base;
>> -} else if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) {
>> +if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) {
>>  decoded_frame->pts = decoded_frame->pkt_pts;
>>  decoded_frame_tb   = ist->st->time_base;
>>  } else if (pkt->pts != AV_NOPTS_VALUE) {
>> --
>
> Ping.

Last chance for further comments, otherwise I'll apply in the morning
(~8 hours from now), so that merges can continue.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[wm4]

On Sat,  1 Oct 2016 16:15:45 +0200
Hendrik Leppkes  wrote:

> Decoders have previously not used AVFrame.pts, and with the upcoming
> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
> interpration of timestamps.
> ---
>  ffmpeg.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 9a8e65a..cdbf3d4 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -2058,12 +2058,7 @@ static int decode_audio(InputStream *ist, AVPacket 
> *pkt, int *got_output)
>  }
>  }
>  
> -/* if the decoder provides a pts, use it instead of the last packet pts.
> -   the decoder could be delaying output by a packet or more. */
> -if (decoded_frame->pts != AV_NOPTS_VALUE) {
> -ist->dts = ist->next_dts = ist->pts = ist->next_pts = 
> av_rescale_q(decoded_frame->pts, avctx->time_base, AV_TIME_BASE_Q);
> -decoded_frame_tb   = avctx->time_base;
> -} else if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) {
> +if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) {
>  decoded_frame->pts = decoded_frame->pkt_pts;
>  decoded_frame_tb   = ist->st->time_base;
>  } else if (pkt->pts != AV_NOPTS_VALUE) {

Seems fine to me. No decoder should ever have set the pts field
before. (Forgot to reply earlier.)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

On Sat, Oct 1, 2016 at 4:15 PM, Hendrik Leppkes  wrote:
> Decoders have previously not used AVFrame.pts, and with the upcoming
> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
> interpration of timestamps.
> ---
>  ffmpeg.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 9a8e65a..cdbf3d4 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -2058,12 +2058,7 @@ static int decode_audio(InputStream *ist, AVPacket 
> *pkt, int *got_output)
>  }
>  }
>
> -/* if the decoder provides a pts, use it instead of the last packet pts.
> -   the decoder could be delaying output by a packet or more. */
> -if (decoded_frame->pts != AV_NOPTS_VALUE) {
> -ist->dts = ist->next_dts = ist->pts = ist->next_pts = 
> av_rescale_q(decoded_frame->pts, avctx->time_base, AV_TIME_BASE_Q);
> -decoded_frame_tb   = avctx->time_base;
> -} else if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) {
> +if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) {
>  decoded_frame->pts = decoded_frame->pkt_pts;
>  decoded_frame_tb   = ist->st->time_base;
>  } else if (pkt->pts != AV_NOPTS_VALUE) {
> --

Ping.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[Hendrik Leppkes]

Decoders have previously not used AVFrame.pts, and with the upcoming
deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
interpration of timestamps.
---
 ffmpeg.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/ffmpeg.c b/ffmpeg.c
index 9a8e65a..cdbf3d4 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -2058,12 +2058,7 @@ static int decode_audio(InputStream *ist, AVPacket *pkt, 
int *got_output)
 }
 }
 
-/* if the decoder provides a pts, use it instead of the last packet pts.
-   the decoder could be delaying output by a packet or more. */
-if (decoded_frame->pts != AV_NOPTS_VALUE) {
-ist->dts = ist->next_dts = ist->pts = ist->next_pts = 
av_rescale_q(decoded_frame->pts, avctx->time_base, AV_TIME_BASE_Q);
-decoded_frame_tb   = avctx->time_base;
-} else if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) {
+if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) {
 decoded_frame->pts = decoded_frame->pkt_pts;
 decoded_frame_tb   = ist->st->time_base;
 } else if (pkt->pts != AV_NOPTS_VALUE) {
-- 
2.10.0.windows.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel