This fixes the compilation:
https://phabricator.kde.org/D20957

Hm, I meanwhile noticed that playback doesn't seem to work at all with vlc 2.2.6 anymore (works fine with 2.0.16)...

So maybe it's better to just drop support for vlc < 3 completely?

(In reply to Wolfgang Bauer from comment #1)
> This fixes the compilation:
> https://phabricator.kde.org/D20957

Thanks!

(In reply to Wolfgang Bauer from comment #2)
> Hm, I meanwhile noticed that playback doesn't seem to work at all with vlc
> 2.2.6 anymore (works fine with 2.0.16)...
> 
> So maybe it's better to just drop support for vlc < 3 completely?

Fixing it should probably be trivial. Either I forgot to add an event used on vlc 2 to notify about playback status changes or we need to restore the diff changes at makePlay() if vlc < 3. This was removed from makePlay, because it was not required with vlc 3 (and/or broke something - don't remember the exact issue I found on that time):

 int VlcMediaWidget::makePlay()
 {
        if (vlcMedia == NULL) {
@@ -343,18 +578,7 @@ int VlcMediaWidget::makePlay()
                return -1;
        }
 
-       libvlc_event_manager_t *eventManager = libvlc_media_event_manager(vlcMedia);
-       libvlc_event_e eventTypes[] = { libvlc_MediaMetaChanged };
-
-       for (uint i = 0; i < (sizeof(eventTypes) / sizeof(eventTypes[0])); ++i) {
-               if (libvlc_event_attach(eventManager, eventTypes[i], vlcEventHandler, this) != 0) {
-                       qCWarning(logMediaWidget, "Cannot attach event handler %d", eventTypes[i]);
-               }
-       }
-
        libvlc_media_player_set_media(vlcMediaPlayer, vlcMedia);
-       libvlc_media_release(vlcMedia);
-       vlcMedia = NULL;

(In reply to Wolfgang Bauer from comment #2)
> Hm, I meanwhile noticed that playback doesn't seem to work at all with vlc
> 2.2.6 anymore (works fine with 2.0.16)...
> 
> So maybe it's better to just drop support for vlc < 3 completely?

I guess the real question is: it is worth keep supporting vlc < 3?

VLC < 3 has a problem with H-264 parser, with causes it to not work properly with MPEG-TS: the problem is that it doesn't properly find the start of an H.264 frame. When a TV channel is switched, it is unlikely to get the start of an H.264 frame: it will generally get in the middle. In practice, one has to try several times to setup a channel until VLC recognizes the stream.

Also, it doesn't support new types of encoding, like H-265.

Without H-264/H-265, lots of DVB channels can't be received, as nowadays most transponders have channels with higher compression standards than MPEG2 (and, on some parts of the globe, all channels are encoded with at least H-264).

From a more practical standpoint, I don't test Kaffeine anymore with vlc 2, as the distros I use already moved to vlc3 a long time ago. Also, here, all terrestrial channels use H.264. I can still simulate plain old MPEG-2, but only via my RF generator. Ok, I guess I could try to setup some environment here to allow testing with an older distro or forcing it to get vlc2 from a different directory. 

Is it worth?

(In reply to Mauro Carvalho Chehab from comment #5)
> Ok, I guess I could try... forcing it to get vlc2 from a different directory. 

That won't work:

configure: error: libavutil versions 55 and later are not supported.

(In reply to Mauro Carvalho Chehab from comment #5)
> (In reply to Wolfgang Bauer from comment #2)
 
> From a more practical standpoint, I don't test Kaffeine anymore with vlc 2,
> as the distros I use already moved to vlc3 a long time ago.

Found a RPi3 here with an old image that still has vlc 2.26. I'll give it a try, and see how easy/hard would be to fix it.

(In reply to Mauro Carvalho Chehab from comment #5)
> VLC < 3 has a problem with H-264 parser, with causes it to not work properly
> with MPEG-TS: the problem is that it doesn't properly find the start of an
> H.264 frame. When a TV channel is switched, it is unlikely to get the start
> of an H.264 frame: it will generally get in the middle. In practice, one has
> to try several times to setup a channel until VLC recognizes the stream.

Well, my DVB card doesn't support DVB-S2 (only DVB-S), so I can't tell.

> Also, it doesn't support new types of encoding, like H-265.

It should support H.265 via the libavcodec plugin I think.

> Is it worth?

Well, I don't know.

The only reason I stumbled over this is that openSUSE Leap 42.3 only has vlc 2.2.x, and I updated openSUSE's kaffeine package yesterday in the (optional) KDE:Extra repo that is also available for 42.3.
OTOH, that's probably not really a big problem, as Leap 42.3 will be EOL in less than 2 months anyway.

(In reply to Mauro Carvalho Chehab from comment #4)
> Fixing it should probably be trivial. Either I forgot to add an event used
> on vlc 2 to notify about playback status changes or we need to restore the
> diff changes at makePlay() if vlc < 3. This was removed from makePlay,
> because it was not required with vlc 3 (and/or broke something - don't
> remember the exact issue I found on that time):
> 
>  int VlcMediaWidget::makePlay()
>  {
>         if (vlcMedia == NULL) {
> @@ -343,18 +578,7 @@ int VlcMediaWidget::makePlay()
>                 return -1;
>         }
>  
> -       libvlc_event_manager_t *eventManager =
> libvlc_media_event_manager(vlcMedia);
> -       libvlc_event_e eventTypes[] = { libvlc_MediaMetaChanged };
> -
> -       for (uint i = 0; i < (sizeof(eventTypes) / sizeof(eventTypes[0]));
> ++i) {
> -               if (libvlc_event_attach(eventManager, eventTypes[i],
> vlcEventHandler, this) != 0) {
> -                       qCWarning(logMediaWidget, "Cannot attach event
> handler %d", eventTypes[i]);
> -               }
> -       }
> -
>         libvlc_media_player_set_media(vlcMediaPlayer, vlcMedia);
> -       libvlc_media_release(vlcMedia);
> -       vlcMedia = NULL;
That patch didn't help. Reverting https://cgit.kde.org/kaffeine.git/commit/?id=d03abc77ad40a3ca25b011c865183f1b6ddf8f87 completely fixed it though.

But, I now noticed that I actually get an error message in konsole:
This object event manager doesn't know about 'MediaMetaChanged' events03-05-19 08:42:00.086 [Critical] kaffeine.mediawidget: Cannot attach event handler 0

So obviously libvlc 2.2 doesn't like the MediaMetaChanged event being attached in the constructor.
This patch would fix playback with libvlc 2.2:
@@ -170,7 +170,6 @@ VlcMediaWidget::VlcMediaWidget(QWidget *parent) : AbstractMediaWidget(parent),
     typeOfDevice(""), trackNumber(1), numTracks(1)
 {
 	libvlc_event_e events[] = {
-		libvlc_MediaMetaChanged,
 		libvlc_MediaPlayerEncounteredError,
 		libvlc_MediaPlayerEndReached,
 		libvlc_MediaPlayerLengthChanged,

(although it should probably be attached again in makePlay() then I suppose)

Full patch that fixes playback with libvlc 2.2.6 and shouldn't affect 3.0+:
@@ -170,7 +170,9 @@ VlcMediaWidget::VlcMediaWidget(QWidget *parent) : AbstractMediaWidget(parent),
     typeOfDevice(""), trackNumber(1), numTracks(1)
 {
 	libvlc_event_e events[] = {
+#if LIBVLC_VERSION_MAJOR > 2
 		libvlc_MediaMetaChanged,
+#endif
 		libvlc_MediaPlayerEncounteredError,
 		libvlc_MediaPlayerEndReached,
 		libvlc_MediaPlayerLengthChanged,
@@ -517,6 +490,13 @@ int VlcMediaWidget::makePlay()
 		return -1;
 	}
 
+#if LIBVLC_VERSION_MAJOR <= 2
+	libvlc_event_manager_t *eventManager = libvlc_media_event_manager(vlcMedia);
+	if (libvlc_event_attach(eventManager, libvlc_MediaMetaChanged, vlcEventHandler, this) != 0) {
+			qCWarning(logMediaWidget, "Cannot attach event handler %d", libvlc_MediaMetaChanged);
+	}
+#endif
+
 	libvlc_media_player_set_media(vlcMediaPlayer, vlcMedia);
 
 	/*

I haven't noticed problems with 2.2.6, tried to play an AudioCD, VideoDVD, AVI (Xvid) file, and DVB-S (MPEG2) tv channels (also recording and playing back the recorded file works). AudioCD track changes are notified too (they weren't without the change to makePlay()).
This obviously leaks the eventManager though, I suppose I can look into trying to fix that too...

(In reply to Wolfgang Bauer from comment #9)
> This obviously leaks the eventManager though, I suppose I can look into
> trying to fix that too...
Or does it?
I'm not completely sure, but maybe the event should be detached again somewhere?

(In reply to Wolfgang Bauer from comment #10)
> (In reply to Wolfgang Bauer from comment #9)
> > This obviously leaks the eventManager though, I suppose I can look into
> > trying to fix that too...
> Or does it?
> I'm not completely sure, but maybe the event should be detached again
> somewhere?

I'm not completely sure either, but it doesn't hurt to explicitly detach it at unregisterEvents().

Did some tests here with RPi3. It seems that it fixed the issue here, although RPi3 is too slow for ISDB-T/H.264 decoding, even for low-res mobile channels.

(In reply to Wolfgang Bauer from comment #8)
> (In reply to Mauro Carvalho Chehab from comment #5)
> > VLC < 3 has a problem with H-264 parser, with causes it to not work properly
> > with MPEG-TS: the problem is that it doesn't properly find the start of an
> > H.264 frame. When a TV channel is switched, it is unlikely to get the start
> > of an H.264 frame: it will generally get in the middle. In practice, one has
> > to try several times to setup a channel until VLC recognizes the stream.
> 
> Well, my DVB card doesn't support DVB-S2 (only DVB-S), so I can't tell.
> 
> > Also, it doesn't support new types of encoding, like H-265.
> 
> It should support H.265 via the libavcodec plugin I think.

It is not a matter of just having support at libav. The libVlc is reponsible to identify the used codecs from the video descriptors within the MPEG Transport Stream. Also, most codecs assume that you're decoding the video from the start of a frame. When you're watching life TV, at the moment a channel is switched, you'll start receiving MPEG-TS frames, receiving data usually in the middle of a frame. It is up to libVlc to identify that, wait until the end of the frame and only after that send the frames for decoding.

The sync logic for H.264 is broken on 2.x (at least last time I tested). I warned VLC developers about that - and I guess I even opened a bug - but didn't receive any feedback about a fix.

I'm almost sure that the descriptors for H.265 (any any other needed sync code) got added only during 3.x development, but it would be possible that someone may have backported to 2.x.

Git commit 402cbee5e675cf795b619b834f92aac086972afc by Mauro Carvalho Chehab.
Committed on 03/05/2019 at 12:11.
Pushed by mauroc into branch 'master'.

vlc: fix support for vlc 2.x

As reported by Wolfgang, the changes made on Kaffeine 2.0.16 to
solve issues with audio CDs broke Kaffeine when building it with
legacy vlc 2.2.x.

The fix here is simple: just place two libVlc 3.0 events at the
print logic inside the Vlc3 block is enough to make it build
again.

The libvlc_MediaMetaChanged is used to report when a new media is
playing.

With vlc 2.0, such event should be enabled only inside mediaPlay()
routine. Doing it early causes the play to not work.

So, re-add such logic, with got removed on changeset d03abc77ad40
("backend-vlc: simplify events handling logic").

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

M  +20   -6    src/backend-vlc/vlcmediawidget.cpp

https://commits.kde.org/kaffeine/402cbee5e675cf795b619b834f92aac086972afc

(In reply to Mauro Carvalho Chehab from comment #12)
> Git commit 402cbee5e675cf795b619b834f92aac086972afc by Mauro Carvalho Chehab.
> Committed on 03/05/2019 at 12:11.
> Pushed by mauroc into branch 'master'.
> 
> vlc: fix support for vlc 2.x

It seems that change was not fully correct.

Playback works now, but I still get this error message:
This object event manager doesn't know about 'MediaMetaChanged' events04-05-19 10:05:54.006 [Warning ] kaffeine.mediawidget: Cannot attach event handler 0

(only problem I noticed is that AudioCD track changes are not notified to the user anymore, there could be more though)

I note that the 2.0.16 code in makePlay() called libvlc_media_event_manager(vlcMedia) to get the eventManager, while the code in the constructor calls libvlc_media_player_event_manager(vlcMediaPlayer).
And it seems the latter doesn't know about libvlc_MediaMetaChanged in vlc 2.x, which I assume is also the reason that playback didn't work at all in the first place...

Created attachment 119837 [details]
Further changes to address issues with vlc 2.x

(In reply to Wolfgang Bauer from comment #13)
> (In reply to Mauro Carvalho Chehab from comment #12)
> > Git commit 402cbee5e675cf795b619b834f92aac086972afc by Mauro Carvalho Chehab.
> > Committed on 03/05/2019 at 12:11.
> > Pushed by mauroc into branch 'master'.
> > 
> > vlc: fix support for vlc 2.x
> 
> It seems that change was not fully correct.
> 
> Playback works now, but I still get this error message:
> This object event manager doesn't know about 'MediaMetaChanged'
> events04-05-19 10:05:54.006 [Warning ] kaffeine.mediawidget: Cannot attach
> event handler 0
> 
> (only problem I noticed is that AudioCD track changes are not notified to
> the user anymore, there could be more though)
> 
> I note that the 2.0.16 code in makePlay() called
> libvlc_media_event_manager(vlcMedia) to get the eventManager, while the code
> in the constructor calls libvlc_media_player_event_manager(vlcMediaPlayer).
> And it seems the latter doesn't know about libvlc_MediaMetaChanged in vlc
> 2.x, which I assume is also the reason that playback didn't work at all in
> the first place...

Gah, that looks a very confusing code at 2.x... Both vlcMedia and vlcMediaPlayer uses the same events enum and shares the same event handler, but it seems they actually have a different set of events, instead of sharing a common event list!

Could you please check if the enclosed patch address the issues? 

I ended by adding a longer comment trying to explain the issue. That should help to keep this code working if we need to do other changes there.

Also, in a matter of fact, we could simply remove the #if, as the code should also work fine on 3.x, but I opted to keep them, as this would make easier if some day we decide to remove support for 2.x.

(In reply to Mauro Carvalho Chehab from comment #15)
> Could you please check if the enclosed patch address the issues? 
Yes, it indeed seems to be fine now.
The playback (still) works, the mentioned error message is gone, AudioCD track changes are now announced again in Kaffeine's window.

Git commit 2644cbf02ed39bb6b194bc6b7935b069b5769364 by Mauro Carvalho Chehab.
Committed on 04/05/2019 at 12:35.
Pushed by mauroc into branch 'master'.

vlc: with vlc 2.x, use vlcMedia for libvlc_MediaMetaChanged event

As reported by Wolfgang, the libVlc 2.x fix applied at
402cbee5e675 ("vlc: fix support for vlc 2.x") still have issues,
causing some troubles at least on audio CDs.

The root cause seems to be that, on vlc 2.x, the event
libvlc_MediaMetaChanged should be registered against the
vlcMedia instance, instead of vlcMediaPlayer instance.

Document that inside the code, as this is not obvious.

While the old way is still supported on vlc 3.x, I opted to
keep the code inside #ifs, as some day we'll drop support for
vlc 2.x, making the code simpler.

Also, since this event is registered aganst a vlcMedia object
and behaves different than when applied against a vlcMediaPlayer
object, I'm also using a different name there, in order to avoid
confusion.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

M  +16   -4    src/backend-vlc/vlcmediawidget.cpp

https://commits.kde.org/kaffeine/2644cbf02ed39bb6b194bc6b7935b069b5769364