[09/12] movdec: Ignore sample_degradation_priority bits when checking first_sample_flags

diff mbox

This makes the first packet of a track fragment run to get
the keyframe flag set properly if sample_degradation_priority
is nonzero.
---
 libavformat/mov.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Fri, 20 Jan 2012, Martin Storsjö wrote:

> This makes the first packet of a track fragment run to get
> the keyframe flag set properly if sample_degradation_priority
> is nonzero.
> ---
> libavformat/mov.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index dd580de..b5964f8 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2251,7 +2251,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>         sc->ctts_data[sc->ctts_count].duration = (flags & 0x800) ? avio_rb32(pb) : 0;
>         sc->ctts_count++;
>         if ((keyframe = st->codec->codec_type == AVMEDIA_TYPE_AUDIO ||
> -             (flags & 0x004 && !i && !sample_flags) || sample_flags & 0x2000000))
> +             (flags & 0x004 && !i && !(sample_flags & 0xffff0000)) || sample_flags & 0x2000000))
>             distance = 0;
>         av_add_index_entry(st, offset, dts, sample_size, distance,
>                            keyframe ? AVINDEX_KEYFRAME : 0);
> -- 
> 1.7.3.1

This is still pending review.

For deciding what's a keyframe, the existing code either checks for 
sample_depends_on == 0, sample_is_depended_on == 0, sample_has_redundancy 
== 0 (which means "unknown") or sample_depends_on == 2, which according to 
the spec means "this sample does not depend on others (I picture)".

The current check for the unknown case in the current form (!sample_flags) 
also requires the sample_degradation_priority field to be zero. E.g. in 
ismv files encoded by MS Expression Encoder 4, this field is nonzero.

So I guess I can amend the commit message with this:
---8<---
This makes the keyframes flag be set properly for ismv files encoded by MS 
Expression Encoder 4 (among other).
---8<---

// Martin

On Wed, 25 Jan 2012, Martin Storsjö wrote:

> On Fri, 20 Jan 2012, Martin Storsjö wrote:
>
>> This makes the first packet of a track fragment run to get
>> the keyframe flag set properly if sample_degradation_priority
>> is nonzero.
>> ---
>> libavformat/mov.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index dd580de..b5964f8 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -2251,7 +2251,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext 
>> *pb, MOVAtom atom)
>>         sc->ctts_data[sc->ctts_count].duration = (flags & 0x800) ? 
>> avio_rb32(pb) : 0;
>>         sc->ctts_count++;
>>         if ((keyframe = st->codec->codec_type == AVMEDIA_TYPE_AUDIO ||
>> -             (flags & 0x004 && !i && !sample_flags) || sample_flags & 
>> 0x2000000))
>> +             (flags & 0x004 && !i && !(sample_flags & 0xffff0000)) || 
>> sample_flags & 0x2000000))
>>             distance = 0;
>>         av_add_index_entry(st, offset, dts, sample_size, distance,
>>                            keyframe ? AVINDEX_KEYFRAME : 0);
>> -- 
>> 1.7.3.1
>
> This is still pending review.
>
> For deciding what's a keyframe, the existing code either checks for 
> sample_depends_on == 0, sample_is_depended_on == 0, sample_has_redundancy == 
> 0 (which means "unknown") or sample_depends_on == 2, which according to the 
> spec means "this sample does not depend on others (I picture)".
>
> The current check for the unknown case in the current form (!sample_flags) 
> also requires the sample_degradation_priority field to be zero. E.g. in ismv 
> files encoded by MS Expression Encoder 4, this field is nonzero.
>
> So I guess I can amend the commit message with this:
> ---8<---
> This makes the keyframes flag be set properly for ismv files encoded by MS 
> Expression Encoder 4 (among other).
> ---8<---

This patch still isn't reviewed. A user on irc reported 
http://bugzilla.libav.org/show_bug.cgi?id=215 today, and after a closer 
look, I realized this patch fixes this bug, too.

// Martin

On 25/01/12 04:52, Martin Storsjö wrote:
> On Fri, 20 Jan 2012, Martin Storsjö wrote:
> 
>> This makes the first packet of a track fragment run to get
>> the keyframe flag set properly if sample_degradation_priority
>> is nonzero.
>> ---
>> libavformat/mov.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index dd580de..b5964f8 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -2251,7 +2251,7 @@ static int mov_read_trun(MOVContext *c,
>> AVIOContext *pb, MOVAtom atom)
>>         sc->ctts_data[sc->ctts_count].duration = (flags & 0x800) ?
>> avio_rb32(pb) : 0;
>>         sc->ctts_count++;
>>         if ((keyframe = st->codec->codec_type == AVMEDIA_TYPE_AUDIO ||
>> -             (flags & 0x004 && !i && !sample_flags) || sample_flags &
>> 0x2000000))
>> +             (flags & 0x004 && !i && !(sample_flags & 0xffff0000)) ||
>> sample_flags & 0x2000000))
>>             distance = 0;
>>         av_add_index_entry(st, offset, dts, sample_size, distance,
>>                            keyframe ? AVINDEX_KEYFRAME : 0);
>> -- 
>> 1.7.3.1
> 
> This is still pending review.
> 
> For deciding what's a keyframe, the existing code either checks for
> sample_depends_on == 0, sample_is_depended_on == 0,
> sample_has_redundancy == 0 (which means "unknown") or sample_depends_on
> == 2, which according to the spec means "this sample does not depend on
> others (I picture)".
> 
> The current check for the unknown case in the current form
> (!sample_flags) also requires the sample_degradation_priority field to
> be zero. E.g. in ismv files encoded by MS Expression Encoder 4, this
> field is nonzero.
> 
> So I guess I can amend the commit message with this:
> ---8<---
> This makes the keyframes flag be set properly for ismv files encoded by
> MS Expression Encoder 4 (among other).
> ---8<---
> 

Sounds fine. Could we have less magic numbers w/out a macro explaining
them btw =) ? (it can be done on another patch)

lu

On Mon, 30 Jan 2012, Luca Barbato wrote:

> On 25/01/12 04:52, Martin Storsjö wrote:
>> On Fri, 20 Jan 2012, Martin Storsjö wrote:
>> 
>>> This makes the first packet of a track fragment run to get
>>> the keyframe flag set properly if sample_degradation_priority
>>> is nonzero.
>>> ---
>>> libavformat/mov.c |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index dd580de..b5964f8 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -2251,7 +2251,7 @@ static int mov_read_trun(MOVContext *c,
>>> AVIOContext *pb, MOVAtom atom)
>>>         sc->ctts_data[sc->ctts_count].duration = (flags & 0x800) ?
>>> avio_rb32(pb) : 0;
>>>         sc->ctts_count++;
>>>         if ((keyframe = st->codec->codec_type == AVMEDIA_TYPE_AUDIO ||
>>> -             (flags & 0x004 && !i && !sample_flags) || sample_flags &
>>> 0x2000000))
>>> +             (flags & 0x004 && !i && !(sample_flags & 0xffff0000)) ||
>>> sample_flags & 0x2000000))
>>>             distance = 0;
>>>         av_add_index_entry(st, offset, dts, sample_size, distance,
>>>                            keyframe ? AVINDEX_KEYFRAME : 0);
>>> -- 
>>> 1.7.3.1
>> 
>> This is still pending review.
>> 
>> For deciding what's a keyframe, the existing code either checks for
>> sample_depends_on == 0, sample_is_depended_on == 0,
>> sample_has_redundancy == 0 (which means "unknown") or sample_depends_on
>> == 2, which according to the spec means "this sample does not depend on
>> others (I picture)".
>> 
>> The current check for the unknown case in the current form
>> (!sample_flags) also requires the sample_degradation_priority field to
>> be zero. E.g. in ismv files encoded by MS Expression Encoder 4, this
>> field is nonzero.
>> 
>> So I guess I can amend the commit message with this:
>> ---8<---
>> This makes the keyframes flag be set properly for ismv files encoded by
>> MS Expression Encoder 4 (among other).
>> ---8<---
>> 
>
> Sounds fine. Could we have less magic numbers w/out a macro explaining
> them btw =) ? (it can be done on another patch)

That might be a good idea - I find myself jumping frenetically back and 
forth between a few places in the iso media spec when dealing with these 
sample flags :-)

Pushed.

// Martin