[FFmpeg-devel] [PATCH] MMS protocol support patch 1

[Björn Axelsson]

On Sun, 2007-09-30 at 02:03 +0200, Michael Niedermayer wrote:
> Hi
> 
> On Tue, Sep 25, 2007 at 05:51:52PM +0200, Bj?rn Axelsson wrote:
> > On Sat, 2007-09-22 at 02:43 +0200, Michael Niedermayer wrote:
> > > Hi
> > > 
> > > On Wed, Sep 19, 2007 at 05:51:16PM +0200, Bj?rn Axelsson wrote:
> > > > Hi,
> > > > this is the first patch for MMS protocol support. Since the last RFC i
> > > > have cleaned it up overall, split it up into several files and selected
> > > > a minimal functional subset for your reviewing pleasure. I know it is
> > > > still a lot of code to review, but it should at least be better than the
> > > > last monster MMS patch.
> > > > 
> > > > This patch only includes mmsh (MMS over HTTP) support to keep it as
> > > > small as possible. It also appears to be what most servers support. Some
> > > > of the generic mms code make more or less obvious references to the mmst
> > > > (MMS over TCP) protocol, which I hope is ok since that implementation
> > > > will be added in a later patch.
> > > > 
> > > > When (if?) this is accepted I have some separate patches planned:
> > > > - mmst support (implemented, just waiting for the smoke to clear)
> > > > - mms protocol that chooses the best possible of the existing
> > > > subprotocols (either mmst or mmsh)
> > > 
> > > > - clean API for controlling protocols (seek, play, pause).
> > > 
> > > its ok if you cleanup the API after adding MMS support but i will not accept
> > > hacks in the code due to a missing clean API, that is if you can simply
> > > remove/split out the parts which would need API changes then thats fine
> > > otherwise the API cleanup will have to be done first
> > 
> > Ok. Attached is a first draft for the API additions. It passes
> > regression tests and performs well with an adjusted MMS patch.
> > 
> > The goal is to expose a protocol-level API for functionality similar to
> > read_play, read_pause and read_seek in AVInputFormat. The utility of
> > protocol-level play/pause is obvious, and read_seek enables usage of
> > server-side indices for seeking to specific time points.
> > 
> > The highest level API is still av_play(), av_pause() and av_seek_frame()
> > and most applications should require no changes.
> > 
> > There are a couple of weak points I'd like some feedback on, though:
> > 
> > - After a protocol-level seek most demuxers would need to be notified so
> > that they can reset their current state. In the patch I chose the
> > easiest solution: each demuxer need to implement support for
> > protocol-level seeking as I have done in asf.c in the patch. An
> > alternative would be to add a read_flush() or similar call to
> > AVInputFormat and do the protocol-level seeking in av_seek_frame().
> > 
> > - AVInputFormat:read_seek() takes a stream_index parameter which makes
> > little sense for a protocol. Correctly supporting would require the
> > URLProtocol to maintain an AVFormatContext parallel to that of the
> > demuxer with the hope that stream indices are identical in both.
> > Additionally, there's still no guarantee that the actual protocol
> > supports time point seeking in specific streams. The attached patch
> > keeps the stream_index parameter for the URLProtocol:url_read_seek, but
> > allows it to be "-1" for "any stream". A cleaner solution would be to
> > drop that parameter from the URLProtocol and ByteIOContext read_seek,
> > but then their semantics would differ even more from
> > AVInputformat:read_seek and it would be much harder to add support for
> > stream_index in the future.
> > (My MMS implementation does use a "shadow" AVFormatContext, but will
> > still not be able to honor stream_index since it is unknown if it is
> > possible with MMS to select a specific stream when seeking to a time
> > point.)
> > Without the stream_index parameter it would be possible to merge
> > url_read_seek into url_seek with a special whence parameter, e.g.
> > AVSEEK_TIME similar to AVSEEK_SIZE.
> 
> i wish i could give you some feedback on these but iam not sure what is
> best either ...

Having reflected somewhat more on it I believe that demuxer support for
protocol-level seek is best added to each demuxer separately (as done in
the patch). That is least intrusive and keeps the demuxer in control of
its ByteIOContext.

I also think that URLProtocol:url_read_seek() should keep the
stream_index parameter, but allow implementations to return NOTSUPP. In
that case the application or demuxer can recalculate the seek time based
on the stream offset and retry with a -1 stream_index. It would be very
consistent with the demuxer read_seek() call, yet still support
fallbacks when the full implementation isn't possible.

> [...]
> 
> > Index: libavformat/avformat.h
> > ===================================================================
> > --- libavformat/avformat.h	(revision 10573)
> > +++ libavformat/avformat.h	(arbetskopia)
> > @@ -21,8 +21,8 @@
> >  #ifndef AVFORMAT_H
> >  #define AVFORMAT_H
> >  
> > -#define LIBAVFORMAT_VERSION_INT ((51<<16)+(13<<8)+4)
> > -#define LIBAVFORMAT_VERSION     51.13.4
> > +#define LIBAVFORMAT_VERSION_INT ((52<<16)+(0<<8)+0)
> > +#define LIBAVFORMAT_VERSION     52.0.0
> >  #define LIBAVFORMAT_BUILD       LIBAVFORMAT_VERSION_INT
> 
> ahh, great, theres no easy way to avoid that i guess?
> these additions in ByteIOContext and its use in AVFormatContext :(
> maybe we should think about if we at least can correct the code so
> similar extensions in the future wont require a major version bump

I'm not sure what you mean here. I guess that you mean that protocol API
additions need new fields in several structs and break the ABI.

How about implementing something similar to ioctl() for this kind of
functionality?

> [...]
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
-- 
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