[FFmpeg-devel] [PATCH 3/3] [RFC] mpegts: Support seeking based on stream timestamps.

[wm4]

On Sun, 28 Sep 2014 13:37:59 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On 28.09.2014, at 11:05, wm4 <nfxjfg at googlemail.com> wrote:
> > On Sun, 28 Sep 2014 10:40:18 +0200
> > Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> > 
> >> On Sun, Sep 28, 2014 at 10:15:51AM +0200, wm4 wrote:
> >>> On Sun, 21 Sep 2014 10:17:16 +0100
> >>> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> >>>> @@ -2680,6 +2687,7 @@ AVInputFormat ff_mpegtsraw_demuxer = {
> >>>>     .read_packet    = mpegts_raw_read_packet,
> >>>>     .read_close     = mpegts_read_close,
> >>>>     .read_timestamp = mpegts_get_dts,
> >>>> +    .read_seek      = mpegts_read_seek,
> >>>>     .flags          = AVFMT_SHOW_IDS | AVFMT_TS_DISCONT,
> >>>>     .priv_class     = &mpegtsraw_class,
> >>>> };
> >>> 
> >>> IMO this is not a good idea. Seeking should seek the stream to a
> >>> timestamp; but the demuxer will output mismatching timestamps with a
> >>> different offset!
> >> 
> >> If you combine a stream layer with different timestamps, yes.
> > 
> > That's not how the libavformat seeking API works. If you want a
> > different layer, use something from the different layer. E.g. seek avio
> > directly, and flush the demuxer's buffers.
> 
> Which as you mention later is only possible with messy hacks.

All you need is something like av_format_flush(ctx).

> >>> Also, in the
> >>> context of MPlayer, your patch might actually trigger slow operations
> >>> when playing a .ts file with cache enabled: it will have to do a
> >>> synchronous call through the stream cache layer to call the seek
> >>> function.
> >> 
> >> How is that in any way different from your proposal that fixes it
> >> in MPlayer?
> >> Also that operation isn't any slower than normal seeking (assuming
> >> the seek is not within the cache).
> > 
> > Huh? With your patch, MPlayer would send a seek stream ctrl to the
> > cache, and would have to do a blocking wait for the cache process. It
> > can't know whether the implementation is even implemented before
> > sending the stream ctrl.
> 
> That is in no way different from implementing it in MPlayer, unless you compared doing a hack of only calling stream seek if the stream type is bluray but not doing that optimization in the read_seek function.

The difference is that libavformat does NOT know if you're using
libbluray somewhere, while the application does.

On the other hand, handling read_seek might be absolutely required by
someone who implements avio callbacks for use with a demuxer, because
else seeking will break with obscure formats (like rtmp).

So the question is: what do you do if your ts (or anything else) is
actually in a seekable file, and you don't want libavformat to invoke
the read_seek callback? This is not possible without having full
knowledge which formats call read_seek "just because", and which really
need it for working seeking.

> But then the speed difference is because you compare different implementations.
> Also this only applies to cases where the read_seek is not supported.
> In other cases the performance is unchanged.

But this is the more common case...

> Also it only doubles the round-trip latency which if that really is significant is an issue that needs to be fixed at the source.

Probably, but note that MPlayer-style byte caches can be read and
seeked even if the underlying stream is stuck and blocked. And if that
is not an issue, why have a cache at all?

> 
> >> This fixes only one specific single case and it doesn't help an
> >> application that wants to combine its own bluray handling (for example
> >> using something other than libbluray) with FFmpeg's demuxer, they
> >> still need to hack up their own seeking code for that and then
> >> beat FFmpeg it not seeking on its own, and manually reset the demuxer
> >> etc.
> > 
> > Absolutely nothing stops the application from doing its own seeking for
> > formats like mpeg-ts. Though what you need is a function that flushes
> > internal libavformat buffers to make sure no old data is read after the
> > stream-level seek was performed. (Currently I execute a byte seek to
> > the current position to achieve this flushing.)
> 
> And this horribly broken, unreliable, crappy hack is supposed to be a better "solution"?
> The absolute minimum for me to consider that acceptable would be a working, tested, official resync function (we really don't have that yet for demuxers?).

What are you talking about? My MPlayer fork uses libavformat with
libbluray just fine, while MPlayer doesn't and _can't_.

The only "hack" about this is that libavformat doesn't provide a proper
flush function. It could be easily added.

> >> Either way the read_seek is there, and until such time someone removes
> >> it I am not inclined to consider it a good idea to keep it broken!
> > 
> > It's not broken. If the fact that mpeg-ts doesn't use it means it's
> > broken, you're going to have to apply the same patch to a lot of
> > demuxers.
> > 
> > And again, I don't understand why you can't just redirect the seek in
> > demux_lavf.c. This would be an easy fix for the MPlayer problem.
> 
> Because I want FFmpeg to work well, not make both projects a crap heap by adding workarounds in one around issues in the other.

I just suggested a clean solution. If you don't want to listen, fine,
pursue your shitty hacks to keep an aging codebase working.