[FFmpeg-devel] [PATCH 1/1] segafilm: fetch duration from the container

[James Almer]

On 4/20/2018 2:39 AM, Misty De Meo wrote:
> On Thu, Apr 19, 2018 at 8:31 PM, James Almer <jamrial at gmail.com> wrote:
>>
>>> +        film->sample_table[i].duration = AV_RB32(&scratch[12]);
>>
>> While for video tracks this field seems to report the same packet
>> durations that were being calculated pre patch, this field for audio
>> tracks is always 1.
>>
>> Before this patch a codec copy of the sample logo-capcom.cpk in the FATE
>> suite gave:
>>
>> 1,          0,          0,    22048,    44096, 0xafd250ae
>> 0,          2,          2,        2,    11080, 0xac3a462b
>> 0,          4,          4,        2,    11300, 0xd8ee7f3e
>> 0,          6,          6,        2,    21612, 0x73c3a3f9, F=0x0
>> 0,          8,          8,        2,    21628, 0x00a5b4b9, F=0x0
>> 0,         10,         10,        2,    14772, 0x1332b44f
>> 0,         12,         12,        2,    14744, 0x5ce5d59b
>> 0,         14,         14,        2,    14736, 0xd5ac2877
>> 1,      22048,      22048,    11028,    22056, 0xe08a0f01
>>
>> Whereas after applying it becomes:
>>
>> 1,          0,          0,        1,    44096, 0xafd250ae
>> 0,          2,          2,        2,    11080, 0xac3a462b
>> 0,          4,          4,        2,    11300, 0xd8ee7f3e
>> 0,          6,          6,        2,    21612, 0x73c3a3f9, F=0x0
>> 0,          8,          8,        2,    21628, 0x00a5b4b9, F=0x0
>> 0,         10,         10,        2,    14772, 0x1332b44f
>> 0,         12,         12,        2,    14744, 0x5ce5d59b
>> 0,         14,         14,        2,    14736, 0xd5ac2877
>> 1,      22048,      22048,        1,    22056, 0xe08a0f01
>>
>> For reference, decoding it always gives the following with or without
>> this patch:
>>
>> 0,          0,          0,        1,   215040, 0x067c5362
>> 1,          0,          0,    22048,    88192, 0x23bb50ae
>> 0,          2,          2,        1,   215040, 0xd9eacb98
>> 0,          4,          4,        1,   215040, 0x3c8a4cbd
>> 0,          6,          6,        1,   215040, 0xbdf996e1
>> 0,          8,          8,        1,   215040, 0x1b7fa123
>> 0,         10,         10,        1,   215040, 0x834b4a8d
>> 0,         12,         12,        1,   215040, 0xf4b1bebe
>> 0,         14,         14,        1,   215040, 0x088c3802
>> 1,      22048,      22048,    11028,    44112, 0x79600f01
>>
>> Is this change desired/intended?
> 
> This is actually intended; since that value is what's in the original
> container, it's what should get passed through, especially for stream
> copying back into a new FILM file.

Maybe the muxer should ignore the input packet duration when writing
this field in that case, and hardcode it to 1. A duration of 1 for an
audio track with these presentation times is not correct.

You should set this value only for video. If pkt->duration is not set,
libavformat will calculate it on its own.

> 
>>
>> Also, unrelated to this patch, but as you can see decoding outputs one
>> extra video frame at the beginning that codec copy doesn't. this is
>> because packet keyframes are being set wrong: A 0 in the top bit of the
>> sample info dword on each sample table entry signals an Intra coded
>> frame, whereas 1 signals an Inter coded frame. The demuxer is assuming
>> the opposite.
> 
> Here's a thread with context on the change to the intra setting:
> http://ffmpeg.org/pipermail/ffmpeg-devel/2018-March/227014.html
> 
> The keyframe logic was intentionally inverted in
> cfe1a9d311de6c36641cf295004cdbc77d7b600c

I don't understand the reasoning to invert the logic here. The current
behavior is clearly not right. We're marking Inter coded frames as key
frames and Intra coded frames as non keyframes. The result as you can
see is at least one frame/packet lost during codec copy.

Setting pkt->flags was the only change that patch should have introduced.