Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DVB channel switching broken #2568

Closed
olifre opened this issue Dec 6, 2015 · 19 comments
Closed

DVB channel switching broken #2568

olifre opened this issue Dec 6, 2015 · 19 comments

Comments

@olifre
Copy link
Member

olifre commented Dec 6, 2015

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:
a609877

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
Copy link

ghost commented Dec 6, 2015

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
Copy link
Member Author

olifre commented Dec 6, 2015

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
Copy link

ghost commented Dec 6, 2015

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

@olifre
Copy link
Member Author

olifre commented Dec 6, 2015

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
Copy link

ghost commented Dec 6, 2015

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
Copy link
Member Author

olifre commented Dec 6, 2015

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
Copy link

ghost commented Dec 6, 2015

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
Copy link
Member Author

olifre commented Dec 6, 2015

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
Copy link

ghost commented Dec 7, 2015

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
Copy link
Member Author

olifre commented Dec 7, 2015

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
Copy link

paulguy commented Dec 7, 2015

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
Copy link

ghost commented Dec 10, 2015

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

@paulguy
Copy link

paulguy commented Dec 10, 2015

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
Copy link

ghost commented Dec 10, 2015

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
Copy link
Member Author

olifre commented Jan 7, 2016

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 pushed a commit that referenced this issue Jan 7, 2016
PT_RELOAD_FILE is a somewhat obscure case when using DVB or when
switching Matroska editions. Both cases were broken, because the
asynchronous playback abort mechanism was still triggered. This
mechanism is used to force the demuxer and stream layers to exit
immediately (instead of blocking on I/O possibly forever), and
is normally disabled on playback start. The reopen path is a bit
strange, and needs to reset it manually.

Pointed out in #2568.
@ghost
Copy link

ghost commented Jan 7, 2016

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

@olifre
Copy link
Member Author

olifre commented Jan 7, 2016

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
Copy link
Member Author

olifre commented Jan 8, 2016

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
Copy link

ghost commented Jan 8, 2016

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.

@ghost ghost closed this as completed Jan 13, 2016
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants