DVB channel switching broken

[olifre]

I was just made aware of the problem that DVB channel switching does not work anymore with current mpv. A bisect shows the following commit broke it: a609877f00889a5cb5fe3e4e2877eec49cc90ab0

A gist from running mpv and switching a channel at that commit looks like: https://gist.github.com/olifre/79758fa0c4473a4a4931 It seems lavf does not fully attempt to redetect everything, I don't see a second max-analyze-duration being reached etc.

Since I am unsure how to fix, but have the hardware to test: Please tell me any further information that could help!

[ghost]

From what I can see, what it did on dvb channel switch before that commit was:

• close demuxer
• reopen demuxer

Now it does:

• close demuxer
• close stream
• open stream
• open demuxer

[olifre]

Mhhhm... I don't really see why that should go wrong, but from my gist you can find from the first tuning: demux: Trying demuxer: lavf (force-level: request) lavf: Found 'mpegts' at score=100 size=0 (forced). ffmpeg/demuxer: mpegts: parser not found for codec none, packets or times may be invalid. ffmpeg/demuxer: mpegts: parser not found for codec dvb_teletext, packets or times may be invalid. ffmpeg: libzvbi_teletextdec: page filter: * While after channel switch: lavf: Found 'mpegts' at score=100 size=0 (forced). ffmpeg/demuxer: mpegts: Could not detect TS packet size, defaulting to non-FEC/DVHS lavf: av_find_stream_info() failed cplayer: Failed to recognize file format. I don't see why on the second attempt, i.e. after switching, it does not re-analyze the new stream contents, but bails out so fast...

[ghost]

I think it might have to do with destroying the stream state.

[olifre]

Sorry, but I am still unsure on "where to look". From what I understand,

• close demuxer
• close stream => dvbin_close()
• open stream => dvb_open()
• open demuxer

should do exactly the same as happens during mpv startup, which is also what seems to happen according to the gist... Calling "dvbin_close" when closing the stream should not break things, it cleans up (as one would expect) the channel config and closes all FDs of the devices. Do I need to do anything special to stream_t in dvbin_close to reset the state?

[ghost]

No, the stream gets completely destroyed. I'm not sure why it wouldn't just work. Maybe you could confirm with valgrind that the player code isn't doing anything stupid. Then all what's left is state within the kernel, I suppose?

[olifre]

There's also another more fundamental issue I just found in the gist...

• First startup on channel SUPER RTL, all fine
• cycle, flags=9, args=[dvb-channel, (NULL)] is called, initiating channel switch
• DVB_STEP_CHANNEL dir 1 is called, switching channel to DVB_SET_CHANNEL: new channel name=VOX, card: 0, channel 7
• After the switch has happened, mpv closes demuxer and stream...
• The next "OPEN_DVB: prog=SUPER RTL, card=1, type=1" is done with the "old" channel SUPER RTL which was current before the switch!

I.e. the channel switch does not actually work, which is actually expected, since the channel name and channel list are remembered in the stream which is now destroyed and recreated just after successful channel switching.

So we have two problems here:

• Channel switching does not work since state is remembered in the stream (what's the best place to remember the current channel? Is it really a good idea to re-parse the channel list on each channel switch, which is the case right now since it's remembered in the stream?) From a quick look, I don't see why PVR should not be affected by this issue, also there chan_idx is remembered in a struct in the stream - maybe channel switching has been broken there, too? If that's the case, maybe it's better to leave the stream state intact for these special streams which have somewhat more heavy state information.
• Even then, I would expect we re-tune to the current channel, here I'll try valgrind as you suggested.

[ghost]

Yes, PVR is probably also broken then.

I see some possible solutions:

• somehow keep the stream instance again (I really don't like this, and would be complicated/intrusive)
• somehow add a new abstraction for "persistent" stream state, which is queried before closing the stream, and then set again when opening the stream (this would behave like a value type; how exactly this is done would have yet to be determined)
• keep the real DVB state (including device FD) somewhere else, and make the dvb stream a thin wrapper around it; this way it won't matter that the stream is destroyed (how this state is accessed and where the handle to it is stored / how a new dvb stream gets access to it would have yet to be determined)
• do not actually reload stream and demuxer, but instead make demuxer and player capable of dealing with a changing underlying transport stream (this could be anything from very simple to very complicated - I considered this solution for handling dvdnav, but just ended up removing dvdnav)
• add some state like current channel to the MPOptions struct, just like it's done for e.g. for the current Matroska edition (least painful solution)

[olifre]

I agree - my problem is that at the moment I have no development time, there might be some time around christmas / new year when I can probably contribute a bit...

Concerning the options you listed:

somehow keep the stream instance again (I really don't like this, and would be complicated/intrusive)

I agree, that would not really be a good "solution".

somehow add a new abstraction for "persistent" stream state, which is queried before closing the stream, and then set again when opening the stream (this would behave like a value type; how exactly this is done would have yet to be determined)

Since the full "persistent" stream state might be a complex structure, I am not sure it will be easy to find a generic solution that works well for all kinds of streams which need some kind of persistency.

keep the real DVB state (including device FD) somewhere else, and make the dvb stream a thin wrapper around it; this way it won't matter that the stream is destroyed (how this state is accessed and where the handle to it is stored / how a new dvb stream gets access to it would have yet to be determined)

I think I am in favor of this option. At least for DVB, channel switching may mean opening and closing of several FDs (the demuxes for the several stream-IDs), while some may be reused (the device itself e.g.). Also, there is the channel list, the tuning state etc. In some cases, retuning might not even be needed. To catch all these complications, a persistent DVB state kept extra from the stream state would be most useful, since the DVB implementation part should know best what can be kept and what needs to be redone on channel switch. In the end this is also mostly as it was done before with the persistent stream object, but with better information separation. Do you have a suggestion on how / where to store this "global", "persisten" DVB state information? Note that since I do not have PVR hardware, I can not do that kind of implementation for PVR...

do not actually reload stream and demuxer, but instead make demuxer and player capable of dealing with a changing underlying transport stream (this could be anything from very simple to very complicated - I considered this solution for handling dvdnav, but just ended up removing dvdnav)

That would be the nicest solution, probably, and - if a working way is found - come with the least overhead e.g. on channel switching. But I see it's the most complicated one...

add some state like current channel to the MPOptions struct, just like it's done for e.g. for the current Matroska edition (least painful solution)

I think this will not be sufficient to get a good user experience. If we only remember the current channel number, we lose all other state - reopening the device, re-parsing thousands of lines of channel list, re-tuning, re-extracting information from the live stream from the transponder etc. takes a significant amount of time, so at least for DVB, it would be best to be able to keep "more state".

[ghost]

Do you have a suggestion on how / where to store this "global", "persisten" DVB state information?

Not really. It just has to be dragged along somehow.

That would be the nicest solution, probably, and - if a working way is found - come with the least overhead e.g. on channel switching. But I see it's the most complicated one...

Yes, the problem is that the set of streams changes.

But we also could try to make the stream switching required in this context part of the playback logic. So that means the stream would somehow notify the player that it has to select a new video/audio stream to get data. Isn't it actually so that there are well-defined mechanisms for transport streams how to do that? (And it could happen midstream too. E.g. isn't it allowed that even the video codec changes during receiving?)

Note that since I do not have PVR hardware, I can not do that kind of implementation for PVR...

Don't worry about that. Probably nobody uses that anyway.

[olifre]

Not really. It just has to be dragged along somehow.

Ok - I'll have a look as soon as I have time, which may be still a few weeks ahead. The first step would be to separate out all state information which can (and should) be kept around, then maybe this can be kept in a set of structs (as it's done now) which are kept around for full process lifetime (since DVB is unique, maybe just static pointers to these structs which are null at first and then initialized during first DVB initialization with channel list, open FDs etc.).

Isn't it actually so that there are well-defined mechanisms for transport streams how to do that? (And it could happen midstream too. E.g. isn't it allowed that even the video codec changes during receiving?)

All these funny things are allowed, but I also do not know the details. ffmpeg does a lot of magic here: https://ffmpeg.org/doxygen/2.8/mpegts_8c_source.html And some of that magic is per-packet, and they may even add streams there....

[paulguy]

I do have to say that the linux kernel PVR support is barely worth supporting. Most of the devices in this group are analog standard def MPEG2 encoder cards. Only modern device that works with it is the hauppauge HDPVR and I've dug around in the kernel code for this specific device and the generic PVR code and it's kind of hacked together to "just work" and not much else. Almost like a proof of concept, but is far too unstable and lacking in robustness to use in practice. I'd honestly say, if it gets in the way, drop support for it altogether.

[ghost]

Looks like I'll just remove pvr:// then.

[paulguy]

You can always just use it through adjusting the parameters with the command line v4l2 utilities and opening the /dev/videoN device as a file. Handling mid-stream changes to video codec and resolution and so on would possibly fix some of the problems that PVR ends up having, though. MythTV I hear is fairly reliable with PVR devices so maybe looking in to what they do to handle changes in the MPEG-TS stream would be useful.

[ghost]

Transport streams have well-defined mechanisms to handle such mid-stream changes, but I suppose neither mpv nor libavformat handle them correctly in all cases.

[olifre]

I am currently looking at this. Apart from the issues with keeping DVB-state (which I still need to look into - I don't have a good idea on where to keep it yet...) I think I found the reason MPV terminates now on channel-switch, unless I misunderstand the code...

In https://github.com/mpv-player/mpv/blob/master/player/loadfile.c#L1253 I see the cancel_trigger is fired after reaching EOF, which is also the codepath we enter on channel-switch. Then, everything is uninit'ed. Then, for DVB, we enter the PT_RELOAD_FILE case: https://github.com/mpv-player/mpv/blob/master/player/loadfile.c#L1267 However, the mp_cancel is not reset in between. As such, on reinitializing the demux, it finds the cancel-flag and terminates playback.

So, if I do:

    if (mpctx->stop_play == PT_RELOAD_FILE) {
        mpctx->stop_play = KEEP_PLAYING;
        mp_cancel_reset(mpctx->playback_abort);
        goto reopen_file;
    }

i.e. add an mp_cancel_reset inside, mpv does not exit, but restarts the stream as expected. Channel switching still does not work since the stream is reinitialized with the old channel again, however, before solving this, I thought I'd ask whether this is a bug (or I am missing something)?

[ghost]

Yes, it's a bug. It also broke Matroska edition switching.

[olifre]

There's one more, slightly separate issue which I found. The dvb-channel property is an int-pair consisting of CARD and CHANNEL number. This is perfect in the sense that it allows to switch the channel. It's not so good for querying e.g. the current channel name. Also the media-title is not updated after switch, and MPV always shows "dvb-channel (error)" on switching, probably because there is no value-printing for int-pairs.

Do you have a proposal on this?

• Would it be best to expose an additional read-only property, say "dvb-channel-name", which exposes the current channel? It should not be writable, since technically one should be able to specify card+channel for switching. I don't think anybody uses mpv with several DVB cards, but who knows. It could of course also be a writable property with the documented limitation of not switching the card...
• How do I update the media-title with the current channel name from within stream-dvb? Maybe that information is sufficient then, and we don't need an additional dvb-channel-name?

[olifre]

How do I update the media-title with the current channel name from within stream-dvb? Maybe that information is sufficient then, and we don't need an additional dvb-channel-name?

I think I found out: Apparently I need to implement stream_ctrl_get_metadata for stream_dvb, and in there I should talloc an mp_tag structure and push it into the arg? Memory is then "owned" / stolen by the enclosing cache instance? If that's the correct way, I can implement that tomorrow, that's probably sufficient and an additional dvb-channel-name may not be needed (unless there is a good usecase for that).

Finally let me say (as a C++ developer, normally) mpv code feels really readable and well organized (even more so the core parts). Even though I get to work a bit here just about only once a year, I usually get into it quickly.

[ghost]

Would it be best to expose an additional read-only property, say "dvb-channel-name", which exposes the current channel? It should not be writable,

Sounds ok. That's what properties are for.

It could of course also be a writable property with the documented limitation of not switching the card...

Sounds also ok.

How do I update the media-title with the current channel name from within stream-dvb? Maybe that information is sufficient then, and we don't need an additional dvb-channel-name?

Apparently I need to implement stream_ctrl_get_metadata for stream_dvb, and in there I should talloc an mp_tag structure and push it into the arg? Memory is then "owned" / stolen by the enclosing cache instance?

This would probably be the cleanest.