[FFmpeg-devel] [PATCH][0/4]: MMS base and MMSH implementation

[Michael Niedermayer]

On Fri, Mar 07, 2008 at 01:48:08PM +0100, Bj?rn Axelsson wrote:
> On Thu, 2008-03-06 at 22:44 +0100, Michael Niedermayer wrote:
> > On Thu, Mar 06, 2008 at 03:53:09PM +0100, Bj?rn Axelsson wrote:
> > [...]
> 
> Thanks for your speedy response. I'd like to ask for a few
> clarifications before I start on the next version of the patch.
> 
> > > +/** Private MMS data. */
> > > +typedef struct {
> > 
> > > +    const AVClass *av_class;       ///< Context for logging.
> > 
> > this does belong in URLWhatever not in your private context
> 
> That would require an API change and a major version bump. Is that
> acceptable and wanted?

no, :(
but a AVClass under #ifdef VERSION >= next major could be added


> 
> > > +    MMSProtocol *protocol;          ///< Function pointers for the subprotocol.
> > > +    void *priv_data;                ///< Private data for subprotocol.
> 
> [...]
> 
> > Iam also somewhat opposed to MMSProtocol *protocol. But that is rater minor.
> 
> With only one subprotocol, it doesn't make much sense. But with mmsh,
> mmst and possibly even mmsu I believe 
> 
> mms->protocol->foo(mms)
> 
> is a cleaner solution than sprinkling the shared code with a bunch of 
> 
> if(mms->protocol == MMS_HTTP)
>   mms_http_foo(mms)
> else if(mms->protocol == MMS_TCP)
>   mms_tcp_foo(mms)

well, you still have
if(mms->protocol == MMS_HTTP)
    mms->protocol->foo= mms_http_foo;
else if(mms->protocol == MMS_TCP)
    mms->protocol->foo= mms_tcp_foo;

The reason why i dont like these pointers is that it adds another layer
of indirection a reader has to go through (where is that pointer set and to
what before he can look up the code of the function)


> 
> Although I'm open for other suggestions.
> 
> > > +
> > > +    /** @name ASF Header
> > > +     * Internal handling of the ASF header. The ASF header is stored by the
> > > +     * protocol to facilitate seeking into it. The protocol also parses the
> > > +     * header to get the packet size and elementary stream information.
> > > +     */
> > 
> > why would we want to seek into the header?
> 
> That is surely up to the calling application. IIRC mplayer does this for
> some reason.

If mms itself doesnt support it then i dont see why we should emulate this.
just fail or return zero bytes (unless this really doesnt work ...)


> 
> > > +    /*@{*/
> > > +    uint8_t *asf_header;            ///< ASF header buffer.
> > > +    int asf_header_size;            ///< Size of stored ASF header.
> > > +    int asf_header_read_pos;        ///< Header read offset. See read_packet().
> > > +    int header_parsed;              ///< Flag: header has been fully parsed.
> > > +    AVFormatContext private_av_format_ctx; ///< Private header data (generic).
> > > +    ByteIOContext   private_av_format_pb;  ///< Private header data (IO buf).
> > > +    ASFContext      asf_context;           ///< Private header data (ASF).
> > 
> > private of what? we have an asf demuxer (which you shouldnt touch) a 
> > subprotocol (mmsh), this mms stuff, an underlaying protocol (tcp/http)..
> > is tha asf stuff here the one actually demuxing things or some private
> > seperate thing unrelated to the demuxer used to help avoid code duplication.
> > All this should be clearly documented.
> 
> The protocol needs to parse the streamed ASF header to get info about
> the available elementary streams. Is it ok (but still ugly) to call the
> ASF demuxer for this? 
> The alternatives I can think of is to either reimplement a ASF header
> parser (very bad idea) or to directly call the private functions of the
> ASF demuxer.

Its ok to open a asf demuxer, parse the header, extract what you need and
close it again.
Its not ok to keep the AVFormatContext around to confuse me :)


[...]
> 
> > [...]
> > > +
> > > +extern AVInputFormat asf_demuxer;
> > 
> > This is ugly besides that it should be #include not such copy and
> > paste hacks of what you need from the header.
> 
> Which header would that be? "AVInputFormat asf_demuxer" is never
> declared in any header, and is only used once in allformats.c (with an
> extern declaration).

Once upon a time there was allformats.h ... well it seems somone killed it :(

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20080307/d0b6ca0a/attachment.pgp>