Modify
Warning Please read the Ticket HowTo before creating or commenting on a ticket. Failure to do so may cause your ticket to be rejected or result in a slower response.

Opened 3 years ago

Closed 2 years ago

#9824 closed Patch - Bug Fix (Fixed)

[PATCH] MythPlayer: Fix playback of dvb-s radio channels

Reported by: Lawrence Rust <lvr@…> Owned by:
Priority: minor Milestone: unknown
Component: MythTV - Video Playback Version: 0.24-fixes
Severity: medium Keywords:
Cc: Ticket locked: no

Description

Playback of DVB-S radio channels is broken in several ways:

  • Switching to a radio channel can take considerable time and often results in failure.
  • After the switch the screen often shows a part faded OSD info or a green background.
  • If the channel contains interactive TV (e.g. BBC radio) then OSD elements can't be seen so OSD info for channel change elements are hidden
  • The data rate of some channels is low (64kb) but RingBuffer::CalcReadAheadThresh? doesn't handle these low rates which results in audio underruns.

These patches

  1. Fix the channel changing & OSD issues.
  2. Fix the RingBuffer? thresholds so that audio underruns are avoided.

Attachments (2)

0040-MythPlayer-Improvements-to-DVB-S-radio-playback.patch (13.6 KB) - added by Lawrence Rust <lvr@…> 3 years ago.
0042-RingBuffer-CalcReadAheadThresh-mods-for-low-bit-rate.patch (4.7 KB) - added by Lawrence Rust <lvr@…> 3 years ago.

Download all attachments as: .zip

Change History (24)

Changed 3 years ago by Lawrence Rust <lvr@…>

Changed 3 years ago by Lawrence Rust <lvr@…>

comment:1 Changed 3 years ago by markk

Lawrence - I've just started looking at these patches.

As a general observation (and this applies to various patches you've submitted, not just these), your patches are difficult to review because they contain various unnecessary and largely cosmetic changes that obscure what you are really trying to achieve. For example, converting locks to use QMutexLocker is fine but put them in a different patch.

Given the potential impact of some of these changes, it would also be much appreciated if you explained what you are doing with various changes.

I'd suggest you try and cut them down to what is absolutely needed.

comment:2 Changed 3 years ago by Github

MythPlayer?: Make killdecoder volatile.

Refs #9824

Branch: master
Changeset: d50eab53a64e4f76b0cffd458fc645be1f429853

comment:3 Changed 3 years ago by markk

ref 7d78b55f2

comment:4 Changed 3 years ago by Lawrence Rust <lvr@…>

As a general observation (and this applies to various patches you've submitted, not just these), your patches are difficult to review because they contain various unnecessary and largely cosmetic changes that obscure what you are really trying to achieve. For example, converting locks to use QMutexLocker is fine but put them in a different patch.

I don't accept that. All the changes contained in these 2 patches are necessary with the exception of the additional log output which is 'cosmetic'. Your example, mutex lockers, are required here to facilitate returns without explicitly calling unlock. They improve readability and enable additional return points.

These, and my 'various patches' that I've submitted are what I use on my live systems here. I submit them because I have found them useful and believe others may benefit. I use a condensed version of my git log to create patches for others to download and submit the same patches here. Each patch is intended to stand by itself and so sometimes must be complex as here.

I would comment the code and changes more but I notice a distinct lack of such in the existing code base so I have kept with tradition. If you look at my own projects on www.softsystem.co.uk you will see a much greater level of annotation.

Perhaps these notes will help:

MythPlayer::DisplayNormalFrame?
Only call videoOutput->StartDisplayingFrame?() if there's valid video to display otherwise when tuning to a radio station you just see a green matte screen.

MythPlayer::PreProcessNormalFrame? - commented in the patch

MythPlayer::VideoLoop? - calls DisplayNormalFrame?(!noVideoTracks) i.e. check_prebuffer iff there's video.

MythPlayer::JumpToProgram? - add some error logging and remove a time wasting call to ChangeSpeed?. This sppeds up LiveTV channel changing too.

MythPlayer::EventLoop? - refactor some code for performance and to avoid a potential NULL deref

MythPlayer::PauseDecoder? and MythPlayer::UnpauseDecoder? are re-written to prevent deadlock if pausing audio only streams.

MythPlayer::DecoderPauseCheck? changed to match PauseDecoder?

MythPlayer::DecoderStart? - reduce # retries in case the decoder thread is stuck in AvFormatDecoder::GetFrame?

TV::ChangeChannel? - move PauseAudioUntilBuffered?(ctx) after the video is started. This reduces noise and clicks on restarting radio streams.

RingBuffer::CalcReadAheadThresh? - adjust thresholds for slow streams.

RingBuffer::run - handle situation where readblocksize < CHUNk i.e. slow streams

RingBuffer::WaitForAvail? - this is the most dangerous change as it allows read to return a smaller than demanded size. This is necessary for audio only to prevent audio buffer underruns.

These changes should be tested on 64 kbps audio streams e.g. radio caroline on astra 2d.

HTH

comment:5 Changed 3 years ago by Github

RingBuffer?: Set the minimum raw bitrate to 64Kb.

A single audio track could be as low as 64Kb (or lower?) and MHEG only
channels (i.e. off air) are usually reported as 0Kb (which is probably
not far from the truth).

Ref #9824

Branch: master
Changeset: a1022c6f508c326026ba5101e5a41fd764651659

comment:6 Changed 3 years ago by Github

OSD: Don't clear the interactive screen when the OSD is shown.

This may not be optimal (i.e. the screen may get a little busy) but I'll
see how it goes first. The alternative is to start adding some state
management to ensure the interactive screen is redrawn once the
menu/info has been cleared.

Ref #9824

Branch: master
Changeset: 3e10bc7e7194d83e94412f309ef2420c106e641f

comment:7 Changed 3 years ago by Github

DVB Radio: Fix deinterlacing of dummy video streams.

Dummy frames are generated as progressive but when copied to the
player's video buffers the interlacing flags were not being copied
across and we were unncecessarily processing them.

Ref #9824

Branch: master
Changeset: e763b85d72d094928eb0f12fbd80771eb552ebb7

comment:8 Changed 3 years ago by Github

MythPlayer?: Add a GetFreeVideoFrames? method.

This will be used by AvFormateDecoder::GenerateDummyVideoFrame? to assess
how many dummy video buffers are needed.

Refs #9824

Branch: master
Changeset: 9b39022943e41bba002bc90e686598dfc1100159

comment:9 Changed 3 years ago by Github

DVB Radio: Refactor generation of dummy video frames.

  • Fill the video buffers completely on each call instead of creating one

frame for each call to AvFormatDecoder::GetFrame?. Audio or MHEG only
streams may have very low bitrates and the player will otherwise be
starved of buffers.

  • Don't create a dummy frame, just ask the player to clear the returned

frame. This will allow the VideoOutput? method to clear frames in GPU
memory OR just ignore the dummy frame when it is not required for
display (e.g. VDPAU, VAAPI, OpenGL, DXVA2).

Ref #9824

Branch: master
Changeset: 15d65ae2c41d44f037d1b934d0e6750939f22bb3

comment:10 Changed 3 years ago by Github

VideoFrame?: Add a dummy flag.

This will be used by certain video output methods to skip uploading and
rendering of the dummy video frame.

Ref #9824

Branch: master
Changeset: 5d6c20ff2e6d15f442694b9352c1503aba10718a

comment:11 Changed 3 years ago by Github

MHEG: Produce dummy frames even if there is no MHEG support.

Should fix a few issues for users without MHEG support compiled who tune
to an off air channel (DVB-S/T).

Refs #9824

Branch: master
Changeset: aa44cb6175e612e6503f8777e0ed30ab8aaabf0e

comment:12 Changed 2 years ago by beirdo

  • Owner set to markk
  • Status changed from new to assigned

comment:13 Changed 2 years ago by markk

  • Milestone changed from unknown to 0.25
  • Status changed from assigned to accepted

comment:14 Changed 2 years ago by Github

RingBuffer?: Low bitrate optimisations.

This adds a special case operation for low bitrate streams (e.g. dvb
radio and MHEG/interactive only) where the existing fill requirements
were high enough that, for live tv and in-progress recordings, the
buffer would not fill in time and playaback could and would fail.

I've tried to ensure this is triggered only as needed but there is still
the possibility that faulty bitrate detection may enable it (though even
in that case there should, fingers crossed, be no adverse effects).

There are still at least 2 other problems playing these very low.
Initial stream scanning of MHEG only streams can take over 20 seconds -
as the initial probe still requires 32Kb of data - and stopping playback
is problematic as the decoder thread is not exiting correctly.

Refs #9824

Branch: master
Changeset: d207a0bddddac4cb015746e2a4b4e5f841897c8c

comment:15 Changed 2 years ago by Github

MythPlayer?: Pause the decoder before stopping it.

This ensures the decoder exits cleanly. Vastly improves channel change
time when switching away from a radio or interactive only channel (or
between radio channels) or speeds up exiting playback.

Refs #9824

Branch: master
Changeset: e78269ac292429bb494ba3537a47a8061f58a9f3

comment:16 Changed 2 years ago by Github

Playback: Improved triggers for low bitrate optimisations.

The optimisations added in d207a0bddddac are being triggered
unnecessarily. This is due to a combination of different initial
thresholds in that commit and lowering the acceptable bitrate in
a1022c6f508c3.

The fix is twofold:-

  • adjust the initial bitrate thresholds to ensure any optimisations are

only triggerred for streams with a bitrate below 500Kb/s.

forced to 500Kb/s. This is an extension of the previous hack for H.264
streams.

Overall, this ensures that the previous behaviour (initial fill size of
32KB, rising as needed) is preserved for anything with a video stream
and generally in the vast majority of cases.

Fixes recent playback of avi files and, amongst others, BB HD (both
detected as zero bitrate).

Refs #9824.

Branch: master
Changeset: f6d8c69a9652e5c47618253fe768844211396e6a

comment:17 Changed 2 years ago by markk

comment:18 Changed 2 years ago by markk

  • Owner markk deleted
  • Status changed from accepted to assigned

comment:19 Changed 2 years ago by beirdo

  • Component changed from MythTV - General to MythTV - Video Playback
  • Milestone changed from 0.25 to unknown
  • Status changed from assigned to new

Has the reported problem been sufficiently solved with these many commits referring to it?

comment:20 Changed 2 years ago by beirdo

  • Status changed from new to infoneeded_new

comment:21 Changed 2 years ago by Lawrence Rust <lvr@…>

Mark has significantly improved performance with low bit rate and radio streams so the problems originally described are largely fixed. There are still a few remaining issues with high latency (http) streams and given that my original patches are now very out of date, it may be best to close this ticket and I'll re-submit updated patches with a description of the remaining issues.

comment:22 Changed 2 years ago by mdean

  • Resolution set to Fixed
  • Status changed from infoneeded_new to closed

Closing at reporter's request. Remaining issues will be handled in another ticket.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'new'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.