[FFmpeg-devel] [PATCH] oggdec: validate VP8 keyframes

[James Almer]

On 09/01/14 10:58 PM, Michael Niedermayer wrote:
> On Thu, Jan 09, 2014 at 10:54:04PM -0300, James Almer wrote:
>> On 09/01/14 10:37 PM, Ronald S. Bultje wrote:
>>> Hi,
>>>
>>> On Thu, Jan 9, 2014 at 5:16 PM, James Almer <jamrial at gmail.com> wrote:
>>>
>>>> Fixes seeking in files with broken files
>>>>
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>>  libavformat/oggdec.c | 12 ++++++++++--
>>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
>>>> index efb8f04..9200432 100644
>>>> --- a/libavformat/oggdec.c
>>>> +++ b/libavformat/oggdec.c
>>>> @@ -722,8 +722,16 @@ static void ogg_validate_keyframe(AVFormatContext *s,
>>>> int idx, int pstart, int p
>>>>  {
>>>>      struct ogg *ogg = s->priv_data;
>>>>      struct ogg_stream *os = ogg->streams + idx;
>>>> -    if (psize && s->streams[idx]->codec->codec_id == AV_CODEC_ID_THEORA) {
>>>> -        if (!!(os->pflags & AV_PKT_FLAG_KEY) != !(os->buf[pstart] &
>>>> 0x40)) {
>>>> +    int invalid = 0;
>>>> +    if (psize) {
>>>> +        switch (s->streams[idx]->codec->codec_id) {
>>>> +        case AV_CODEC_ID_THEORA:
>>>> +            invalid = !!(os->pflags & AV_PKT_FLAG_KEY) !=
>>>> !(os->buf[pstart] & 0x40);
>>>> +        break;
>>>> +        case AV_CODEC_ID_VP8:
>>>> +            invalid = !!(os->pflags & AV_PKT_FLAG_KEY) !=
>>>> !(os->buf[pstart] & 1);
>>>> +        }
>>>> +        if (invalid) {
>>>>              os->pflags ^= AV_PKT_FLAG_KEY;
>>>>              av_log(s, AV_LOG_WARNING, "Broken file, %skeyframe not
>>>> correctly marked.\n",
>>>>                     (os->pflags & AV_PKT_FLAG_KEY) ? "" : "non-");
>>>>
>>>
>>> Isn't this duplicate with vp8_parser.c, or in other words, shouldn't we
>>> invoke the AVParser if it exists?
>>>
>>> Ronald
>>
>> I thought the vp8 parser was being invoked by setting st->need_parsing? oggparsevp8 
>> is doing that currently.
>> Nonetheless, the vp8 parser marks I and P frames, but it doesn't seem to set the 
>> AV_PKT_FLAG_KEY flag for the packet (That's done by the demuxer).
>>
>> I did the same thing we're doing for Theora here, fixing wrongly marked keyframes 
>> before sending them to the decoder. It fixed seeking with such broken files for me.
> 
> parse_packet() sets the AVPacket keyframe flags from the parser

So the parser is not being invoked as is? How to do it, then? And why is it not being 
done with Theora as well to avoid calling ogg_validate_keyframe() at all?

Regards.