[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