[FFmpeg-devel,3/6] lavc/libx265: mark disposable frames

Submitted by John Stebbins on Dec. 1, 2017, 12:22 a.m.

Details

Message ID 20171201002209.24270-2-jstebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Dec. 1, 2017, 12:22 a.m.
Used by movenc to fill sdtp box
---
 libavcodec/libx265.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Marton Balint Dec. 2, 2017, 9:40 a.m.
On Thu, 30 Nov 2017, John Stebbins wrote:

> Used by movenc to fill sdtp box
> ---
> libavcodec/libx265.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
> index 4456e300f2..c137fe4ae1 100644
> --- a/libavcodec/libx265.c
> +++ b/libavcodec/libx265.c
> @@ -329,6 +329,9 @@ FF_DISABLE_DEPRECATION_WARNINGS
> FF_ENABLE_DEPRECATION_WARNINGS
> #endif
> 
> +    if (x265pic_out.sliceType == X265_TYPE_B)
> +        pkt->flags |= AV_PKT_FLAG_DISPOSABLE;

You can only set this for B pictures which are not referenced by other 
frames, no? Otherwise they can't be dropped independently from the others.

Regards,
Marton
John Stebbins Dec. 2, 2017, 4:52 p.m.
On 12/02/2017 01:40 AM, Marton Balint wrote:
> On Thu, 30 Nov 2017, John Stebbins wrote:
>
>> Used by movenc to fill sdtp box
>> ---
>> libavcodec/libx265.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
>> index 4456e300f2..c137fe4ae1 100644
>> --- a/libavcodec/libx265.c
>> +++ b/libavcodec/libx265.c
>> @@ -329,6 +329,9 @@ FF_DISABLE_DEPRECATION_WARNINGS
>> FF_ENABLE_DEPRECATION_WARNINGS
>> #endif
>>
>> +    if (x265pic_out.sliceType == X265_TYPE_B)
>> +        pkt->flags |= AV_PKT_FLAG_DISPOSABLE;
> You can only set this for B pictures which are not referenced by other 
> frames, no? Otherwise they can't be dropped independently from the others.
>
>

Correct.  x265 version 2.5 and above returns X265_TYPE_BREF for frames that are referenced.  There is a way to
distinguish on version 2.4 and below as well using x265pic_out.frameData.sliceType, and an earlier version of my patch
used that.  But it's really not the "right" way going forward.  I can restore it if supporting earlier versions of x265
is important.  But my feeling with such fast moving projects as x265 is it's best not to keep too much cruft in support
of old versions.
James Almer Dec. 2, 2017, 5:25 p.m.
On 12/2/2017 1:52 PM, John Stebbins wrote:
> On 12/02/2017 01:40 AM, Marton Balint wrote:
>> On Thu, 30 Nov 2017, John Stebbins wrote:
>>
>>> Used by movenc to fill sdtp box
>>> ---
>>> libavcodec/libx265.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
>>> index 4456e300f2..c137fe4ae1 100644
>>> --- a/libavcodec/libx265.c
>>> +++ b/libavcodec/libx265.c
>>> @@ -329,6 +329,9 @@ FF_DISABLE_DEPRECATION_WARNINGS
>>> FF_ENABLE_DEPRECATION_WARNINGS
>>> #endif
>>>
>>> +    if (x265pic_out.sliceType == X265_TYPE_B)
>>> +        pkt->flags |= AV_PKT_FLAG_DISPOSABLE;
>> You can only set this for B pictures which are not referenced by other 
>> frames, no? Otherwise they can't be dropped independently from the others.
>>
>>
> 
> Correct.  x265 version 2.5 and above returns X265_TYPE_BREF for frames that are referenced.  There is a way to
> distinguish on version 2.4 and below as well using x265pic_out.frameData.sliceType, and an earlier version of my patch
> used that.  But it's really not the "right" way going forward.  I can restore it if supporting earlier versions of x265
> is important.  But my feeling with such fast moving projects as x265 is it's best not to keep too much cruft in support
> of old versions.

You can bump the minimum required version. Just make the necessary
changes in configure.
John Stebbins Dec. 2, 2017, 5:37 p.m.
On 12/02/2017 09:25 AM, James Almer wrote:
> On 12/2/2017 1:52 PM, John Stebbins wrote:
>> On 12/02/2017 01:40 AM, Marton Balint wrote:
>>> On Thu, 30 Nov 2017, John Stebbins wrote:
>>>
>>>> Used by movenc to fill sdtp box
>>>> ---
>>>> libavcodec/libx265.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
>>>> index 4456e300f2..c137fe4ae1 100644
>>>> --- a/libavcodec/libx265.c
>>>> +++ b/libavcodec/libx265.c
>>>> @@ -329,6 +329,9 @@ FF_DISABLE_DEPRECATION_WARNINGS
>>>> FF_ENABLE_DEPRECATION_WARNINGS
>>>> #endif
>>>>
>>>> +    if (x265pic_out.sliceType == X265_TYPE_B)
>>>> +        pkt->flags |= AV_PKT_FLAG_DISPOSABLE;
>>> You can only set this for B pictures which are not referenced by other 
>>> frames, no? Otherwise they can't be dropped independently from the others.
>>>
>>>
>> Correct.  x265 version 2.5 and above returns X265_TYPE_BREF for frames that are referenced.  There is a way to
>> distinguish on version 2.4 and below as well using x265pic_out.frameData.sliceType, and an earlier version of my patch
>> used that.  But it's really not the "right" way going forward.  I can restore it if supporting earlier versions of x265
>> is important.  But my feeling with such fast moving projects as x265 is it's best not to keep too much cruft in support
>> of old versions.
> You can bump the minimum required version. Just make the necessary
> changes in configure.
>

Ah, good point.  That should be done, or I should add back support for earlier versions.  Is there any desire by anyone
to keep support for earlier versions?
Carl Eugen Hoyos Dec. 2, 2017, 5:40 p.m.
2017-12-02 18:37 GMT+01:00 John Stebbins <stebbins@jetheaddev.com>:
> That should be done, or I should add back support for earlier versions.
> Is there any desire by anyone to keep support for earlier versions?

How old is 2.5, is 2.4 used by current versions of distributions?
(How ugly is the support for earlier versions?)

Carl Eugen
James Almer Dec. 2, 2017, 5:48 p.m.
On 12/2/2017 2:37 PM, John Stebbins wrote:
> On 12/02/2017 09:25 AM, James Almer wrote:
>> On 12/2/2017 1:52 PM, John Stebbins wrote:
>>> On 12/02/2017 01:40 AM, Marton Balint wrote:
>>>> On Thu, 30 Nov 2017, John Stebbins wrote:
>>>>
>>>>> Used by movenc to fill sdtp box
>>>>> ---
>>>>> libavcodec/libx265.c | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
>>>>> index 4456e300f2..c137fe4ae1 100644
>>>>> --- a/libavcodec/libx265.c
>>>>> +++ b/libavcodec/libx265.c
>>>>> @@ -329,6 +329,9 @@ FF_DISABLE_DEPRECATION_WARNINGS
>>>>> FF_ENABLE_DEPRECATION_WARNINGS
>>>>> #endif
>>>>>
>>>>> +    if (x265pic_out.sliceType == X265_TYPE_B)
>>>>> +        pkt->flags |= AV_PKT_FLAG_DISPOSABLE;
>>>> You can only set this for B pictures which are not referenced by other 
>>>> frames, no? Otherwise they can't be dropped independently from the others.
>>>>
>>>>
>>> Correct.  x265 version 2.5 and above returns X265_TYPE_BREF for frames that are referenced.  There is a way to
>>> distinguish on version 2.4 and below as well using x265pic_out.frameData.sliceType, and an earlier version of my patch
>>> used that.  But it's really not the "right" way going forward.  I can restore it if supporting earlier versions of x265
>>> is important.  But my feeling with such fast moving projects as x265 is it's best not to keep too much cruft in support
>>> of old versions.
>> You can bump the minimum required version. Just make the necessary
>> changes in configure.
>>
> 
> Ah, good point.  That should be done, or I should add back support for earlier versions.  Is there any desire by anyone
> to keep support for earlier versions?

No, 2.5 is not even the most recent version, so it's fine as the minimum
supported version.
James Almer Dec. 2, 2017, 5:51 p.m.
On 12/2/2017 2:40 PM, Carl Eugen Hoyos wrote:
> 2017-12-02 18:37 GMT+01:00 John Stebbins <stebbins@jetheaddev.com>:
>> That should be done, or I should add back support for earlier versions.
>> Is there any desire by anyone to keep support for earlier versions?
> 
> How old is 2.5, is 2.4 used by current versions of distributions?
> (How ugly is the support for earlier versions?)

It's four months old, and rolling release distros are using it and will
move on to 2.6 soon.

By the time non rolling release distros switch to ffmpeg > 3.4, they
will also switch to whatever is newest for x265 at the time.
Carl Eugen Hoyos Dec. 2, 2017, 6 p.m.
2017-12-02 18:51 GMT+01:00 James Almer <jamrial@gmail.com>:
> On 12/2/2017 2:40 PM, Carl Eugen Hoyos wrote:
>> 2017-12-02 18:37 GMT+01:00 John Stebbins <stebbins@jetheaddev.com>:
>>> That should be done, or I should add back support for earlier versions.
>>> Is there any desire by anyone to keep support for earlier versions?
>>
>> How old is 2.5, is 2.4 used by current versions of distributions?
>> (How ugly is the support for earlier versions?)
>
> It's four months old, and rolling release distros are using it and will
> move on to 2.6 soon.
>
> By the time non rolling release distros switch to ffmpeg > 3.4, they
> will also switch to whatever is newest for x265 at the time.

I was mostly thinking about users who build FFmpeg by themselves
on current distributions. (And about developers who like to test on
vanilla systems.)

Four months seem not a lot to me.

(I believe it is impossible that your criteria ever fails.)

Carl Eugen
James Almer Dec. 2, 2017, 6:26 p.m.
On 12/2/2017 3:00 PM, Carl Eugen Hoyos wrote:
> 2017-12-02 18:51 GMT+01:00 James Almer <jamrial@gmail.com>:
>> On 12/2/2017 2:40 PM, Carl Eugen Hoyos wrote:
>>> 2017-12-02 18:37 GMT+01:00 John Stebbins <stebbins@jetheaddev.com>:
>>>> That should be done, or I should add back support for earlier versions.
>>>> Is there any desire by anyone to keep support for earlier versions?
>>>
>>> How old is 2.5, is 2.4 used by current versions of distributions?
>>> (How ugly is the support for earlier versions?)
>>
>> It's four months old, and rolling release distros are using it and will
>> move on to 2.6 soon.
>>
>> By the time non rolling release distros switch to ffmpeg > 3.4, they
>> will also switch to whatever is newest for x265 at the time.
> 
> I was mostly thinking about users who build FFmpeg by themselves
> on current distributions. (And about developers who like to test on
> vanilla systems.)

Those on rolling release ditros are covered, and those compiling ffmpeg
git head on non rolling release distros are most likely also compiling
any required libraries for it.
Hell, 2.5 is even in Debian testing right now.

I very much rather avoid ifdeffery to support old releases from projects
with a rapid update schedule like x265.
Michael Niedermayer Dec. 3, 2017, 1:48 a.m.
On Sat, Dec 02, 2017 at 03:26:34PM -0300, James Almer wrote:
> On 12/2/2017 3:00 PM, Carl Eugen Hoyos wrote:
> > 2017-12-02 18:51 GMT+01:00 James Almer <jamrial@gmail.com>:
> >> On 12/2/2017 2:40 PM, Carl Eugen Hoyos wrote:
> >>> 2017-12-02 18:37 GMT+01:00 John Stebbins <stebbins@jetheaddev.com>:
> >>>> That should be done, or I should add back support for earlier versions.
> >>>> Is there any desire by anyone to keep support for earlier versions?
> >>>
> >>> How old is 2.5, is 2.4 used by current versions of distributions?
> >>> (How ugly is the support for earlier versions?)
> >>
> >> It's four months old, and rolling release distros are using it and will
> >> move on to 2.6 soon.
> >>
> >> By the time non rolling release distros switch to ffmpeg > 3.4, they
> >> will also switch to whatever is newest for x265 at the time.
> > 
> > I was mostly thinking about users who build FFmpeg by themselves
> > on current distributions. (And about developers who like to test on
> > vanilla systems.)
> 
> Those on rolling release ditros are covered, and those compiling ffmpeg
> git head on non rolling release distros are most likely also compiling
> any required libraries for it.
> Hell, 2.5 is even in Debian testing right now.
> 

> I very much rather avoid ifdeffery to support old releases from projects
> with a rapid update schedule like x265.

I understand why you prefer this but i think its nicer to our users
also the stricter version  deps are the more one runs into issues

for example the "recent" libvpx min version bump required me to
update my libvpx and it promptly broke many older FFmpeg versions
ive patched my release branches locally and that would be in the next
point releases of course but versions released previously require
a libvpx that master doesnt build with and what master builds with
they dont.

If we accumulate too many of these things bisect will become
increasingly painfull

short version, my "vote" is for keeping support for the older version
by #if or any other solution


[...]
James Almer Dec. 3, 2017, 2:32 a.m.
On 12/2/2017 10:48 PM, Michael Niedermayer wrote:
> On Sat, Dec 02, 2017 at 03:26:34PM -0300, James Almer wrote:
>> On 12/2/2017 3:00 PM, Carl Eugen Hoyos wrote:
>>> 2017-12-02 18:51 GMT+01:00 James Almer <jamrial@gmail.com>:
>>>> On 12/2/2017 2:40 PM, Carl Eugen Hoyos wrote:
>>>>> 2017-12-02 18:37 GMT+01:00 John Stebbins <stebbins@jetheaddev.com>:
>>>>>> That should be done, or I should add back support for earlier versions.
>>>>>> Is there any desire by anyone to keep support for earlier versions?
>>>>>
>>>>> How old is 2.5, is 2.4 used by current versions of distributions?
>>>>> (How ugly is the support for earlier versions?)
>>>>
>>>> It's four months old, and rolling release distros are using it and will
>>>> move on to 2.6 soon.
>>>>
>>>> By the time non rolling release distros switch to ffmpeg > 3.4, they
>>>> will also switch to whatever is newest for x265 at the time.
>>>
>>> I was mostly thinking about users who build FFmpeg by themselves
>>> on current distributions. (And about developers who like to test on
>>> vanilla systems.)
>>
>> Those on rolling release ditros are covered, and those compiling ffmpeg
>> git head on non rolling release distros are most likely also compiling
>> any required libraries for it.
>> Hell, 2.5 is even in Debian testing right now.
>>
> 
>> I very much rather avoid ifdeffery to support old releases from projects
>> with a rapid update schedule like x265.
> 
> I understand why you prefer this but i think its nicer to our users
> also the stricter version  deps are the more one runs into issues
> 
> for example the "recent" libvpx min version bump required me to
> update my libvpx

How? 1.4.0 is two years and a half old. Even Debian ships 1.6.x in
stable by now.

> and it promptly broke many older FFmpeg versions
> ive patched my release branches locally and that would be in the next
> point releases of course but versions released previously require
> a libvpx that master doesnt build with and what master builds with
> they dont.
> 
> If we accumulate too many of these things bisect will become
> increasingly painfull

We can't keep external libraries hostage of bisect attempts for
unrelated modules... You don't need libvpx or libx265 compiled in when
hunting down a bug in mpeg2/h264/snow code. We'd never be able to update
anything that way.

I'm surprised for that matter that the libvpx wrappers in old FFMpeg
versions don't work with >= 1.4.0. I don't recall any kind of API break
that we had to work around in them.

> 
> short version, my "vote" is for keeping support for the older version
> by #if or any other solution

I'm fine keeping libx265 as is for now. Unlike libvpx 1.4.0, which is
two years and a half old, libx265 2.5 is admittedly somewhat recent.
But bisecting bugs in unrelated modules is definitely not a reason to
maintain ugly support for old and potentially buggy/unstable/insecure
versions of optional, non system external libraries.
John Stebbins Dec. 3, 2017, 5:16 p.m.
On 12/02/2017 06:32 PM, James Almer wrote:
> On 12/2/2017 10:48 PM, Michael Niedermayer wrote:
>> On Sat, Dec 02, 2017 at 03:26:34PM -0300, James Almer wrote:
>>> On 12/2/2017 3:00 PM, Carl Eugen Hoyos wrote:
>>>> 2017-12-02 18:51 GMT+01:00 James Almer <jamrial@gmail.com>:
>>>>> On 12/2/2017 2:40 PM, Carl Eugen Hoyos wrote:
>>>>>> 2017-12-02 18:37 GMT+01:00 John Stebbins <stebbins@jetheaddev.com>:
>>>>>>> That should be done, or I should add back support for earlier versions.
>>>>>>> Is there any desire by anyone to keep support for earlier versions?
>>>>>> How old is 2.5, is 2.4 used by current versions of distributions?
>>>>>> (How ugly is the support for earlier versions?)
>>>>> It's four months old, and rolling release distros are using it and will
>>>>> move on to 2.6 soon.
>>>>>
>>>>> By the time non rolling release distros switch to ffmpeg > 3.4, they
>>>>> will also switch to whatever is newest for x265 at the time.
>>>> I was mostly thinking about users who build FFmpeg by themselves
>>>> on current distributions. (And about developers who like to test on
>>>> vanilla systems.)
>>> Those on rolling release ditros are covered, and those compiling ffmpeg
>>> git head on non rolling release distros are most likely also compiling
>>> any required libraries for it.
>>> Hell, 2.5 is even in Debian testing right now.
>>>
>>> I very much rather avoid ifdeffery to support old releases from projects
>>> with a rapid update schedule like x265.
>> I understand why you prefer this but i think its nicer to our users
>> also the stricter version  deps are the more one runs into issues
>>
>> for example the "recent" libvpx min version bump required me to
>> update my libvpx
> How? 1.4.0 is two years and a half old. Even Debian ships 1.6.x in
> stable by now.
>
>> and it promptly broke many older FFmpeg versions
>> ive patched my release branches locally and that would be in the next
>> point releases of course but versions released previously require
>> a libvpx that master doesnt build with and what master builds with
>> they dont.
>>
>> If we accumulate too many of these things bisect will become
>> increasingly painfull
> We can't keep external libraries hostage of bisect attempts for
> unrelated modules... You don't need libvpx or libx265 compiled in when
> hunting down a bug in mpeg2/h264/snow code. We'd never be able to update
> anything that way.
>
> I'm surprised for that matter that the libvpx wrappers in old FFMpeg
> versions don't work with >= 1.4.0. I don't recall any kind of API break
> that we had to work around in them.
>
>> short version, my "vote" is for keeping support for the older version
>> by #if or any other solution
> I'm fine keeping libx265 as is for now. Unlike libvpx 1.4.0, which is
> two years and a half old, libx265 2.5 is admittedly somewhat recent.
> But bisecting bugs in unrelated modules is definitely not a reason to
> maintain ugly support for old and potentially buggy/unstable/insecure
> versions of optional, non system external libraries.
>

I'll add back the support for libx265 <= 2.4 then.  There's technically no ifdef'ery required.  But it requires using a
field in a struct (x265_frame_stats) that is really meant for logging.  My concern with using it is that logging could
change in the future and break this. So I'll ifdef it anyway so that it's more future proof.
Michael Niedermayer Dec. 3, 2017, 9:42 p.m.
On Sat, Dec 02, 2017 at 11:32:16PM -0300, James Almer wrote:
> On 12/2/2017 10:48 PM, Michael Niedermayer wrote:
> > On Sat, Dec 02, 2017 at 03:26:34PM -0300, James Almer wrote:
[...]

> I'm surprised for that matter that the libvpx wrappers in old FFMpeg
> versions don't work with >= 1.4.0. I don't recall any kind of API break
> that we had to work around in them.

i dont know about exact versions, i just know some release branches
stoped building with configure settings that worked previously after i
updated libvpx

i needed to backport this:

commit 6540fe04a3f9a11ba7084a49b3ee5fa2fc5b32ab
Author: James Zern <jzern@google.com>
Date:   Mon Oct 19 22:44:11 2015 -0700

    libvpxenc: remove some unused ctrl id mappings

    VP8E_UPD_ENTROPY, VP8E_UPD_REFERENCE, VP8E_USE_REFERENCE were removed
    from libvpx and the remaining values were never used here

    Reviewed-by: Michael Niedermayer <michael@niedermayer.cc>
    Signed-off-by: James Zern <jzern@google.com>

[...]
Hendrik Leppkes Dec. 3, 2017, 10:05 p.m.
On Sun, Dec 3, 2017 at 10:42 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Sat, Dec 02, 2017 at 11:32:16PM -0300, James Almer wrote:
>> On 12/2/2017 10:48 PM, Michael Niedermayer wrote:
>> > On Sat, Dec 02, 2017 at 03:26:34PM -0300, James Almer wrote:
> [...]
>
>> I'm surprised for that matter that the libvpx wrappers in old FFMpeg
>> versions don't work with >= 1.4.0. I don't recall any kind of API break
>> that we had to work around in them.
>
> i dont know about exact versions, i just know some release branches
> stoped building with configure settings that worked previously after i
> updated libvpx
>
> i needed to backport this:
>

That change is kind of the opposite of what we're really talking about
here, isn't it. It provides support for newer versions, didn't remove
support for older ones.

- Hendrik
Michael Niedermayer Dec. 4, 2017, 6:12 p.m.
On Sun, Dec 03, 2017 at 11:05:36PM +0100, Hendrik Leppkes wrote:
> On Sun, Dec 3, 2017 at 10:42 PM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > On Sat, Dec 02, 2017 at 11:32:16PM -0300, James Almer wrote:
> >> On 12/2/2017 10:48 PM, Michael Niedermayer wrote:
> >> > On Sat, Dec 02, 2017 at 03:26:34PM -0300, James Almer wrote:
> > [...]
> >
> >> I'm surprised for that matter that the libvpx wrappers in old FFMpeg
> >> versions don't work with >= 1.4.0. I don't recall any kind of API break
> >> that we had to work around in them.
> >
> > i dont know about exact versions, i just know some release branches
> > stoped building with configure settings that worked previously after i
> > updated libvpx
> >
> > i needed to backport this:
> >
> 
> That change is kind of the opposite of what we're really talking about
> here, isn't it. It provides support for newer versions, didn't remove
> support for older ones.

yes, there was a 2nd change that droped support for older versions

one to add support for newer
then ater
one to drop old

so new doesnt work before both
and
old doesnt work after both
backporting the first fixes this

for refernce:
for the other change "git log" points me to this, so i assume it was
what removed support for old versions

commit b765a04550ff6f954a57bf28158511d260f2e087
Author: James Zern <jzern@google.com>
Date:   Fri Nov 17 17:24:07 2017 -0800

    configure: require libvpx-1.4.0 for vp[89] support

    this will simplify libvpxenc/dec.c and ensure more stable versions of
    the codecs are present.

    Reviewed-by: James Almer <jamrial@gmail.com>
    Signed-off-by: James Zern <jzern@google.com>


[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index 4456e300f2..c137fe4ae1 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -329,6 +329,9 @@  FF_DISABLE_DEPRECATION_WARNINGS
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
 
+    if (x265pic_out.sliceType == X265_TYPE_B)
+        pkt->flags |= AV_PKT_FLAG_DISPOSABLE;
+
     *got_packet = 1;
     return 0;
 }