[FFmpeg-cvslog] r26053 - in trunk: libavformat/asfdec.c tests/ref/fate/wmv8-drm

[Vitor Sessak]

On 12/18/2010 08:55 PM, Reimar D?ffinger wrote:
> On Sat, Dec 18, 2010 at 08:32:41PM +0100, Vitor Sessak wrote:
>> On 12/18/2010 06:19 PM, Reimar D?ffinger wrote:
>>> On Sat, Dec 18, 2010 at 10:43:38AM -0500, Ronald S. Bultje wrote:
>>>> On Sat, Dec 18, 2010 at 8:18 AM, reimar<subversion at mplayerhq.hu>   wrote:
>>>>> Author: reimar
>>>>> Date: Sat Dec 18 14:18:52 2010
>>>>> New Revision: 26053
>>>>>
>>>>> Log:
>>>>> Change ASF demuxer to return incomplete last packets.
>>>>> Whether the behaviour for streams using scrambling makes sense
>>>>> is unclear.
>>>>>
>>>>> Modified:
>>>>>    trunk/libavformat/asfdec.c
>>>>>    trunk/tests/ref/fate/wmv8-drm
>>>>>
>>>>> Modified: trunk/libavformat/asfdec.c
>>>>> ==============================================================================
>>>>> --- trunk/libavformat/asfdec.c  Sat Dec 18 06:15:32 2010        (r26052)
>>>>> +++ trunk/libavformat/asfdec.c  Sat Dec 18 14:18:52 2010        (r26053)
>>>>> @@ -953,12 +953,24 @@ static int ff_asf_parse_packet(AVFormatC
>>>>>
>>>>>          ret = get_buffer(pb, asf_st->pkt.data + asf->packet_frag_offset,
>>>>>                           asf->packet_frag_size);
>>>>> -        if (ret != asf->packet_frag_size)
>>>>> -            return ret>= 0 ? AVERROR_EOF : ret;
>>>>> +        if (ret != asf->packet_frag_size) {
>>>>> +            if (ret<   0 || asf->packet_frag_offset + ret == 0)
>>>>> +                return ret<   0 ? ret : AVERROR_EOF;
>>>>> +            if (asf_st->ds_span>   1) {
>>>>> +                // scrambling, we can either drop it completely or fill the remainder
>>>>> +                // TODO: should we fill the whole packet instead of just the current
>>>>> +                // fragment?
>>>>> +                memset(asf_st->pkt.data + asf->packet_frag_offset + ret, 0,
>>>>> +                       asf->packet_frag_size - ret);
>>>>> +                ret = asf->packet_frag_size;
>>>>> +            } else
>>>>> +                // no scrambling, so we can return partial packets
>>>>> +                av_shrink_packet(&asf_st->pkt, asf->packet_frag_offset + ret);
>>>>> +        }
>>>>>          if (s->key&&   s->keylen == 20)
>>>>>              ff_asfcrypt_dec(s->key, asf_st->pkt.data + asf->packet_frag_offset,
>>>>> -                            asf->packet_frag_size);
>>>>> -        asf_st->frag_offset += asf->packet_frag_size;
>>>>> +                            ret);
>>>>> +        asf_st->frag_offset += ret;
>>>>>          /* test if whole packet is read */
>>>>>          if (asf_st->frag_offset == asf_st->pkt.size) {
>>>>>              //workaround for macroshit radio DVR-MS files
>>>>>
>>>>> Modified: trunk/tests/ref/fate/wmv8-drm
>>>>> ==============================================================================
>>>>> --- trunk/tests/ref/fate/wmv8-drm       Sat Dec 18 06:15:32 2010        (r26052)
>>>>> +++ trunk/tests/ref/fate/wmv8-drm       Sat Dec 18 14:18:52 2010        (r26053)
>>>>> @@ -160,3 +160,4 @@
>>>>>   0, 596250, 84480, 0xbce22331
>>>>>   0, 600000, 84480, 0x020545d7
>>>>>   0, 603750, 84480, 0x71869e48
>>>>> +0, 607500, 84480, 0x5befc578
>>>>
>>>> Various fate platforms fail te wmv8-drm test now, e.g. x86-32 (but
>>>> x86-64 works fine).
>>>
>>> This time unfortunately the issue is indeed that the VC-1 decoder reads outside
>>> the packet size.
>>> Below hack would fix it, but that is of course not ok.
>>> My proposal would be to remove that fate test an only keep the one I added
>>> (does not decode), unless someone comes up with a way to fix the VC-1 decoder...
>>
>> This is not ideal, since the coverage of the VC-1 decoder is far
>> from great in FATE already, and decoding one file less will only
>> make it worse. Can't you re-cut the file to remove the incomplete
>> frame?
>
> Yes (though honestly it's not trivial at all IMO), but then we do not have
> a test-case for neither the incomplete-last-frame nor that combined with
> encryption case.

That's a good point. If we use Michael's suggestion can't we have both?

> Why not add more VC-1 test-case if you think we do not have enough?
> We have some samples, and it is still common enough on e.g. Blu-ray
> (as well as the encoder beeing freely available to any Windows user)
> that it should be possible to get a few more if needed...

Well, ideally, someone who knows the internals of the codec should pick 
a set of samples that cover most of the code...

-Vitor