[Ffmpeg-devel] [PATCH] asf.[ch] patch to handle mms streaming....

Ryan Martell rdm4
Wed Nov 29 01:40:48 CET 2006


Hi...
On Nov 28, 2006, at 5:34 PM, Michael Niedermayer wrote:
> Hi
> On Tue, Nov 28, 2006 at 11:03:27AM -0600, Ryan Martell wrote:
>> Hi...
>> On Nov 28, 2006, at 4:56 AM, Michael Niedermayer wrote:
>>> Hi
>>> On Mon, Nov 27, 2006 at 05:28:00PM -0600, Ryan Martell wrote:
>>>> A couple of minor modifications to handle mms streaming.
>>>>
>>>> mms streaming patch will be forthcoming (if anyone is interested?)
>>>
>>> yes mms support is welcome ...
>>>
>>>
>>> [...]
>>>
>>>> Index: asf.c
>>>> ===================================================================
>>>> --- asf.c	(revision 7177)
>>>> +++ asf.c	(working copy)
>>>> @@ -205,6 +205,11 @@
>>>>             st->start_time = asf->hdr.preroll;
>>>>             st->duration = asf->hdr.send_time /
>>>>                 (10000000 / 1000) - st->start_time;
>>>> +            if(asf->hdr.flags & 0x01) { // streaming; override
>>>> these values
>>>> +                st->duration= AV_NOPTS_VALUE;
>>>
>>> somehow i think this should rather be:
>>> if(!(asf->hdr.flags & 1))
>>>    st->duration = asf->hdr.send_time / (10000000 / 1000) - st-
>>>> start_time;
>>> instead of setting it wrongly and then correcting it ...
>>
>> Well, I was going to do that in a later patch; I was trying to keep
>> the modifications down so that it was easiest to diff. ;-)
>>
>>>> +                asf->nb_packets = asf->hdr.packets_count; //
>>>> MAX_VALUE
>>>
>>> is this not already set to that?
>>
>> It was; this was a different problem that I fixed by changing the
>> signed issue of nb_packets in asf.h, since packets_count is max value
>> on streaming.  Removed.
>>
>>>> +                asf->packet_timestamp_start = -1; // force the
>>>> renumbering of packets (there could be overflow issues here)
>>>> +            }
>>>>             get_guid(pb, &g);
>>>>
>>>>             test_for_ext_stream_audio = 0;
>>>> @@ -333,6 +338,9 @@
>>>>             } else {
>>>>                 asf->data_object_size = (uint64_t)-1;
>>>>             }
>>>> +            if(asf->hdr.flags & 0x01) { // streaming (replace)
>>>> +                asf->data_object_size = (uint64_t)-1;
>>>> +            }
>>>
>>> why not merge this with the else above?
>>
>> Because this happens later in the processing.  Value is now set in
>> the top case, and if'd out here.
>>
>> I was trying to make my changes as small and isolated as possible to
>> make the patch easy to review, with no indentation changes; my
>> original version did the above things!
>>
>> Here's a patch with the above issues changed; note that this required
>> indentation changes, albeit small ones.
>>
>
>> Index: asf.c
>> ===================================================================
>> --- asf.c	(revision 7177)
>> +++ asf.c	(working copy)
>> @@ -203,8 +203,14 @@
>>                  goto fail;
>>              st->priv_data = asf_st;
>>              st->start_time = asf->hdr.preroll;
>> -            st->duration = asf->hdr.send_time /
>> -                (10000000 / 1000) - st->start_time;
>> +            if(!(asf->hdr.flags & 0x01)) { // if not streaming...
>> +                st->duration = asf->hdr.send_time /
>> +                    (10000000 / 1000) - st->start_time;
>> +            } else { // streaming; override these values
>> +                st->duration= AV_NOPTS_VALUE;
>> +                asf->packet_timestamp_start = -1; // force the  
>> renumbering of packets (there could be overflow issues here)
>> +                asf->data_object_size = (uint64_t)-1; //  
>> unlimited size.
>> +            }
>
> st->duration should be at AV_NOPTS_VALUE or 0 by default so it  
> shouldnt
> be needed to set it to that, also the asf->* stuff should IMHO not  
> be set
> in code specific to a specific stream

Okay, I didn't realize there was a previously set default value for  
that.  Done.

Also, I removed the packet_timestamp_start stuff, as it wasn't  
solving the problem I was trying to solve (when running with ffplay,  
the stats shows large numbers, instead of starting at 0 and working  
up by seconds).

By the way, note that in the read_packet stuff, it also sets the  
global structure, not a stream variable.

> and IMHO if(a) else is simpler and more readable the if(!(a)) else

Agreed, but in the latest patch (below), i don't have to do anything  
in the streaming case, so its if(!(a))..

>>              get_guid(pb, &g);
>>
>>              test_for_ext_stream_audio = 0;
>> @@ -328,10 +334,12 @@
>>              url_fskip(pb, gsize - (pos2 - pos1 + 24));
>>          } else if (!memcmp(&g, &data_header, sizeof(GUID))) {
>>              asf->data_object_offset = url_ftell(pb);
>> -            if (gsize != (uint64_t)-1 && gsize >= 24) {
>> -                asf->data_object_size = gsize - 24;
>> -            } else {
>> -                asf->data_object_size = (uint64_t)-1;
>> +            if(!(asf->hdr.flags & 0x01)) { // if not streaming...
>> +                if (gsize != (uint64_t)-1 && gsize >= 24) {
>> +                    asf->data_object_size = gsize - 24;
>> +                } else {
>> +                    asf->data_object_size = (uint64_t)-1;
>> +                }
>>              }
>
> what value does gsize have for streaming? is this special case needed
> at all?

Yes; gsize is the size of just the header for streaming.  I tried  
changing gsize where it gets set at the start, but that didn't work,  
and I settled on this as the cleanest and least likely to break other  
code.

> and maybe you could just replace the data_object_size == -1 cases by
>  == 0 this would avoid haveing to set it to the non-default -1

<rant>
the asf code has all sorts of random stuff in it; variables that are  
assigned to and never read, variables that are read from but not  
really assigned to (except by clearing to zero), etc.   (I found this  
out when I initially just ripped out the read header and the read  
packet for a different approach to mms streaming)

for example, I don't think this is right:
             if((url_ftell(&s->pb) + ret - s->data_offset) % asf- 
 >packet_size)
                 ret += asf->packet_size - ((url_ftell(&s->pb) + ret  
- s->data_offset) % asf->packet_size);
as nothing in this file assigns anything to s->data_offset. I think  
they meant to use asf->data_offset.

Anyway, my goal was/is to make the smallest, simplest changes to the  
code, to make sure I don't break anything for anyone else.  That's  
why i didn't do things like changing the data_object_size default  
cases.  Also note that packet_timestamp_start was a variable that was  
already in the asf header, but was never being used, so i used it for  
setting the timestamp subtraction. (which I  have now removed!)
</rant>

<grin/>

> and last idea, if all else fails can data_object_size at least be  
> set just
> here like:
> if (gsize != (uint64_t)-1 && gsize >= 24 && !(asf->hdr.flags & 1)) {
>     asf->data_object_size = gsize - 24;
> }else{
>     asf->data_object_size = (uint64_t)-1;

I like that better.  Done.

> [...]
>
>> Index: asf.h
>> ===================================================================
>> --- asf.h	(revision 7177)
>> +++ asf.h	(working copy)
>> @@ -86,7 +86,7 @@
>>      int asfid2avid[128];        /* conversion table from asf ID 2  
>> AVStream ID */
>>      ASFStream streams[128];     /* it's max number and it's not  
>> that big */
>>      /* non streamed additonnal info */
>> -    int64_t nb_packets;
>> +    uint64_t nb_packets;
>
> can you exlpain why this change is needed? (yes iam perfectly fine  
> with the
> change unsigned is more correct, iam just curious)

the value of nb_packets is set to the value read from the header.   
for streaming, it's uint64_t max value.  i was reading the header,  
but it wasn't reading any packets, and i figured there was a loop  
somewhere looking at this value.  in reality, it's another one that's  
set and never read from.  So i could take it out, but it is  
technically correct now.

So here's a smaller, tighter patch.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch.txt
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061128/c50d4e17/attachment.txt>
-------------- next part --------------

-Ryan



More information about the ffmpeg-devel mailing list