[FFmpeg-devel] [PATCH] RTSP-MS 15/15: move packet_time_start zero value assignment in asf.c

[Michael Niedermayer]

On Fri, Apr 03, 2009 at 11:15:42AM -0400, Ronald S. Bultje wrote:
> Hi Michael,
> 
> On Fri, Apr 3, 2009 at 10:53 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > what does this patch do and why?
> > and [1] points to something random
> > (yeah ive said in the past that patches and bugs refering to external
> > ?discussion while ommiting a basic 2 sentance description are always
> > ?rejected, this one here is a good example why this is very bad
> > ?practice)
> 
> The ASF demuxer works as follows (pseudo-code):
> 
> read_packet(pkt) {
>   while (1) {
>     if (decode_packet(pkt))
>       break;
>     if (!sync_on_packet_header())
>       assert(some condition);
>     // that one line sits here
>   }
>   return 0;
> }
> 
> sync_on_packet() syncs on an ASF packet, which might contain 0-N
> "useful parsed data packets" (e.g. multiple video frames, or a video
> frame divided over multiple ASF packets, same for audio). When we have
> a complete data unit, it returns.
> 

> The RTP/ASF code does something similar with some slight changes
> because of exact 1 RTP packet == 1 ASF packet mapping. However, we
> don't use read_packet(), but instead directly use the decode_packet()

why do you do this? you know this is VERY sick and VERY poor design, calling
internal demuxer functions from another demuxer ...


> and sync_on_packet_header() functions so that this one line that
> resets something in the ASF header is not called, and thus the ASF
> demuxer state is never reset. This patch moves it to the place where
> all other conditions are reset (in sync_on_packet_header()) so that
> ASF works over RTSP as well.

good, iam now convinced your patch is wrong, and this already was my
feeling from the begin.
The "something" is reset after the function, so it is rest in all cases
of return, not just one like after your patch. I do not know what may
happen if its not reset in some return cases but as you apparenty have
not considered this you cant know it either.


> 
> You've asked in the past why it was needed: so far it wasn't, but now
> it's needed for RTSP. You'll probably next ask why it wasn't there to
> begin with, and my best bet for that is that the ASF demuxer is hacked
> together rather than intelligently designed (take that, evolutionists!
> :-) ). If you had reviewed whoever added this hack in it, you would've
> told them to move it to a place where all other variables were reset
> for the next packet as well. My patch does exactly that (and makes
> RTSP/ASF work).

sorry its your code that is the mess much more than asf.c is
its you who wants to leave the variable in an undefined state in case
of error returns (and it IS used by other functions afterwards)
its you who want to let RT*P call internal asf functions, why do you
not cleanly instantiate a asf demuxer and use it through the common
API that all demuxers use?

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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090403/f2e751d5/attachment.pgp>