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

[Björn Axelsson]

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?

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

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.

> > +    /*@{*/
> > +    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.

Your separate points about bad documentation and unneded context
variables are taken and will be taken care of.

> [...]
> And after reading the code, no you cannot access the demuxer context and
> discard streams based on its AVStream.discard. This is very ugly.
> Also theres no need nor sense or anything else in this.
> The data if it really has to be passed from demuxer to protocol can be
> in an AVDiscard [].

The protocol needs the AVDiscard data from the application before it
requests the multiplexed ASF stream from the server.
I am attempting to do this transparently by getting a pointer to the
application's demuxer context and the AVDiscard values in there and hope
that the application sets the discard values between a read_header() and
the first read() or play(). 
Getting only the AVDiscard[] pointer is slightly less ugly, but there's
still a huge chance of fail. Better ideas are welcome.

I can remove this functionality for now (as the default is to request
all elementary streams from the server), but some kind of solution will
be needed.

> [...]
> > +
> > +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).

-- 
Bj?rn Axelsson                    Phone: +46-(0)90-18 98 97
Intinor AB                          Fax: +46-(0)920-757 10
www.intinor.se
Interactive Television & Digital Media Distribution