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

[Michael Niedermayer]

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

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



>              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?

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

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;


[...]

> 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)

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is