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

AMLCodec: use a fallback rate for invalid fps hints #15667

Merged
merged 1 commit into from Mar 25, 2019

Conversation

Projects
None yet
6 participants
@jahutchi
Copy link
Contributor

commented Mar 4, 2019

Description

Currently, the AMLCodec honours invalid ffmpeg fps hints such as 90,000 which leads to unwatchable playback.

This patch causes the AMLCodec to check the fps rate reported by the decoder api, and use this to update the video rate where this differs from the original rate calculated from the ffmpeg hints. This will also help playback of streams where the frame rate changes part way through.

Motivation and Context

Invalid hints can be passed along in situations where ffmpeg has been unable to determine a correct framerate during FFMPEG analyze duration (e.g. for some tvheadend MPEGTS streams, with multiple streams included).

In such situations, kodi fails to read the framerate:

DEBUG: ffmpeg[DF8822F0]: [mpegts] Could not find codec parameters for stream 0 (Video: h264 ([27][0][0][0] / 0x001B), none): unspecified size
DEBUG: ffmpeg[DF8822F0]: [mpegts] Consider increasing the value for the 'analyzeduration' and 'probesize' options
DEBUG: Open - av_find_stream_info finished
INFO: ffmpeg[DF8822F0]: Input #0, mpegts, from '/mnt/mediapc/tvshows/Emmerdale_2019-02-07_sample.ts':
INFO: ffmpeg[DF8822F0]:   Duration: 00:01:00.42, start: 1.400000, bitrate: 7894 kb/s
INFO: ffmpeg[DF8822F0]:   Program 1
INFO: ffmpeg[DF8822F0]:     Metadata:
INFO: ffmpeg[DF8822F0]:       service_name    : Service01
INFO: ffmpeg[DF8822F0]:       service_provider: FFmpeg
INFO: ffmpeg[DF8822F0]:     Stream #0:0[0x100]: Video: h264 ([27][0][0][0] / 0x001B), none, 90k tbr, 90k tbn, 180k tbc
INFO: ffmpeg[DF8822F0]:     Stream #0:1[0x101](eng): Audio: ac3 ([129][0][0][0] / 0x0081), 48000 Hz, stereo, fltp, 192 kb/s

From my testing I found that if the analyze duration were increased (from 0.5 to 2 seconds), then the fps rate of the problematic files can be determined correctly. However, this idea was considered and rejected, as it would break other use cases: #15612

I then found that my problematic files actually playback OK on other H/W decoders (e.g. VDPAU), so I did some debugging of the AMLCodec...

I found that when the frame rate fails to be determined, this causes a hint of 90,000fps to be passed along to the AMLCodec:

CAMLCodec::OpenDecoder hints.width(1920), hints.height(1080), hints.codec(27), hints.codec_tag(27)
CAMLCodec::OpenDecoder hints.fpsrate(90000), hints.fpsscale(1), video_rate(1), hints.bitsperpixel(8)
CAMLCodec::OpenDecoder hints.aspect(1.777778), video_ratio.num(1), video_ratio.den(1)
CAMLCodec::OpenDecoder hints.orientation(0), hints.forced_aspect(0), hints.extrasize(44)

i.e. fpsrate=90000 fpsscale=1

AMLCodec decoder then uses the following calculation:
am_private->video_rate = 0.5 + (float)UNIT_FREQ * hints.fpsscale / hints.fpsrate;

i.e.
0.5 + 96000 * 90000 / 1

This results in a video_rate of 1! which leads to an unwatchable file. The exact same is true for all the problematic tvheadend recordings which I’ve been keeping hold of (they all result in the decoder hint being set to 90000 fps).

How Has This Been Tested?

With this patch in place I have played back several tvheadend recordings that I have kept hold of, which failed to playback correctly (symptoms described above). In each case, the files now play back correctly.

I also took the time to run through many of the sample clips at various frame rates from the official samples page. In each case, comparing the results before and after the patch. I haven't seen any regressions (also running with the PlayerDebug info on-screen, to watch for any dropped or skipped frames).

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@AlwinEsch AlwinEsch requested a review from peak3d Mar 5, 2019

@AlwinEsch

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Thanks for your first request to us!
I'm not a specialist on that's why I brought only this style comments.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@jahutchi what is am_private->video_rate good for?
For kodi the aml freerun = 2 mode should be used and this one is completely independend from AML internal driver clock.
Why is 90000 unwatchable, but Monitor frequency is?

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

For kodi the aml freerun = 2 mode should be used and this one is completely independend from AML internal driver clock.

Thanks for the tip - on my system (Vero4k+ on OSMC) I can see that /sys/class/video/freerun_mode=1

I shall try setting this to 2 and perform some testing with my problematic test clips.

Why is 90000 unwatchable, but Monitor frequency is?

With a 90,000fps hint the video freezes and drags like crazy. Ive only found this to affect a subset of recordings created by tvhadend. As mentioned, its also possible to fix the issue by increasing the ffmpeg analyze duration from 0.5->2 (though that was rejected #15612), as then the correct framerate is passed along.

I used the monitor refresh rate as a fallback as this seemed a sensible thing to do rather than using some hardcoded value, but open to suggestions.

However, if setting freerun_mode=2 resolves the issue, then i guess this is the way to go.

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@peak3d apologies for the questions but its the first time I've delved into the code in this area.

For kodi the aml freerun = 2 mode should be used and this one is completely independend from AML internal driver clock

I tried setting /sys/class/video/freerun _mode to 2. Only to find it's set back to 1 by kodi when playing back the video. I can see this is done lower down CAMLCodec::OpenDecoder

So are you actually saying that when unreasonable hints are received then it would be better to have kodi set freerun_mode to 2, rather than trying to set some kind of default video rate via the internal driver clock. If so, then it looks like it should be easy enough to adjust the PR.

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@peak3d sorry again... now I look more closely, I see there are two different freerun_mode parameters being set in AMLCodec:

AMLCodec.cpp:1754: SysfsUtils::SetInt("/sys/class/video/freerun_mode", 1);
AMLCodec.cpp:1793: SysfsUtils::SetInt("/sys/module/amlvideodri/parameters/freerun_mode", 3);

Just wondering which of these you are referring to?

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@jahutchi for LibreELEC we made some time ago a kernel patch to allow better video sync inside kodi.
This patch introduced https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDCodecs/Video/AMLCodec.cpp#L1782 and lead to the situation that kodi controls when a frame is released,

In general kodi measures frame rate internaly and Renderer follows this calulated framerate.

The kernel patch we use is here: peak3d/linux@543dc2f
In this branch you'll find some other patches which were used for LE at the time we used AML (LE switched fully to V4L2 without AML, so the patch is not used anymore)

Not sure if kodi runs with AML without these kernel patches, If so, yes, I'm ok with merging the PR because nobody really cares anymore about the AML path.

@kszaq

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

AFAIK OSMC has the kernel patches included.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

If this change helps you in any way (is this approoved? Or just a theoretical PR?), I'm fine to let it go in.
Interesting fact is that current screen refresh rate does not need to match the video fps, so it is wrong as before, no?

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Not sure if kodi runs with AML without these kernel patches, If so, yes, I'm ok with merging the PR because nobody really cares anymore about the AML path.

@peak3d ok thanks, in that case it looks like my kernel does not have those patches; since I can (visually) see that adjusting the fps hints / video_rate in the code has a direct effect on how the frames are being released.

For the problematic tvheadend recordings I find that (without my patch) the video_rate is set to 1
i.e. 0.5 + 96000 * 90000 / 1
....and the video playback continually stutters and freezes.

With my patch in place, it uses the current (gui) refresh rate (50Hz in my case)
e.g. 0.5 + 96000 * 50 / 1
....Resulting in a video_rate of 3844, which gives nice smooth playback.

I have now corrected the code guideline problem pointed out by @AlwinEsch

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

If this change helps you in any way (is this approoved? Or just a theoretical PR?), I'm fine to let it go in.
Interesting fact is that current screen refresh rate does not need to match the video fps, so it is wrong as before, no?

I have my screen refresh rate set to 50hz which matches the refresh rate of the problematic tvheadend recordings. If I switch to 60hz then playback is still soooo much better than when the 90,000fps hint was being honoured.

Point is, this is a fallback, and a fallback is never going to be perfect. Though at least this way you can influence things by adjusting your refresh rate. The ideal solutuon is that the fps rate is always correctly determined by ffmpeg (see #15612 for why this is not always the case). But in cases where it isnt, then at least there is something reasonable to fallback on.

I have sample files i can make available if required.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Question is if it makes sense to set the framerate in decoder at the time kodi has calculated it by the amound of frames. Then it would be a solution and not a workaround.

You should be able to find the log line in kodi log when VideoPlayer has calculated the correct value

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

B.t.w AML codec has already implemented an own framerate tracking:

https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDCodecs/Video/DVDVideoCodecAmlogic.cpp#L348

If it changes you could simply set the correct value to AML

@davilla

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

only for mpeg2 :)

@davilla

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

see https://github.com/MrMC/mrmc/blob/master/xbmc/cores/dvdplayer/DVDCodecs/Video/DVDVideoCodecAVFoundation.mm

It tracks the framerate from dts/pts so the frame presentation time is right. CodecAVFoundation is a bypass codec/renderer. It decodes and renders to a layer under GLES.

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

AFAIK OSMC has the kernel patches included.

Yes it looks like those patches have been in the OSMC kernel for some time:
osmc/vero3-linux@e17e927

You should be able to find the log line in kodi log when VideoPlayer has calculated the correct value

Here are some relevant extracts from a logfile (without my patch applied)

2019-03-07 07:20:48.863 T:3932414704   ERROR: CVideoPlayerVideo::OpenStream - Invalid framerate 90000, using forced 25fps and just trust timestamps
2019-03-07 07:20:48.866 T:3048084208   DEBUG: CAMLCodec::OpenDecoder hints.fpsrate(90000), hints.fpsscale(1), video_rate(1), hints.bitsperpixel(8)
2019-03-07 07:20:49.518 T:3048084208   DEBUG: CRenderManager::Configure - change configuration. 1920x1080. display: 1920x1080. framerate: 25.00.
2019-03-07 07:20:51.884 T:3048084208   DEBUG: CPtsTracker: detected pattern of length 1: 20000.00, frameduration: 20000.000000
2019-03-07 07:20:52.871 T:3048084208   DEBUG: CalcFrameRate framerate was:25.000000 calculated:50.000000
2019-03-07 07:20:52.892 T:3048084208   DEBUG: CRenderManager::Configure - change configuration. 1920x1080. display: 1920x1080. framerate: 50.00.

Question is if it makes sense to set the framerate in decoder at the time kodi has calculated it by the amound of frames. Then it would be a solution and not a workaround.

Any ideas on how this would look / whether it'd require any major re-work.

If this change helps you in any way (is this approoved? Or just a theoretical PR?), I'm fine to let it go in.

It certainly helps me - since around 5-10% of the recordings created by my tvheadend installation suffer from this problem, and are unwatchable. I've tried upgrading tvheadend, fiddling with configuration settings, etc, etc.

Before diving into the code, my only workaround was to create a samba share for my recordings, then hook my phone upto another hdmi port on the tv, and use an app on my phone (ES File Manager) to playback the .ts file.

I'm now starting to wonder whether it'd be best to merge this PR for v18, and look into re-working the code to use the dts/pts and/or the fps rate determined by CalcFrameRate (in those situations where a reasonable framerate hint was received) for v19 - as that sounds like a fair amount of work.

I must admit that until this week I had no knowledge of the VideoPlayer code (quite a learning curve), so I may not be the best person to do a complete re-write to make it use dts/pts tracking. However, I would be more than happy to perform testing and/or or make sample files available in order to help the development of such a long-term solution.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Am I right that your issue only appears for mpeg2 codec streams?

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Am I right that your issue only appears for mpeg2 codec streams?

So far, I've only found the issue to affect mpegts files with the h264 video codec - such as this one:

2019-03-06 20:26:22.989 T:3716150000    INFO: ffmpeg[DD7FF2F0]: Input #0, mpegts, from '/mnt/mediapc/tvshows/Emmerdale_2019-02-07.ts':
2019-03-06 20:26:22.989 T:3716150000    INFO: ffmpeg[DD7FF2F0]:   Duration: 00:35:59.66, start: 79069.492711, bitrate: 7707 kb/s
2019-03-06 20:26:22.989 T:3716150000    INFO: ffmpeg[DD7FF2F0]:   Program 21040
2019-03-06 20:26:22.989 T:3716150000    INFO: ffmpeg[DD7FF2F0]:     Stream #0:0[0x906]: Video: h264 ([27][0][0][0] / 0x001B), none, 90k tbr, 90k tbn, 180k tbc
2019-03-06 20:26:22.990 T:3716150000    INFO: ffmpeg[DD7FF2F0]:     Stream #0:1[0x909](NAR): Audio: mp3 ([4][0][0][0] / 0x0004), 0 channels
2019-03-06 20:26:22.990 T:3716150000    INFO: ffmpeg[DD7FF2F0]:     Stream #0:2[0x91a](eng): Subtitle: dvb_subtitle ([6][0][0][0] / 0x0006)
2019-03-06 20:26:22.990 T:3716150000    INFO: ffmpeg[DD7FF2F0]:     Stream #0:3[0x907](eng): Audio: ac3 (0x05 / 0x35307830), 48000 Hz, stereo, fltp, 192 kb/s
2019-03-06 20:26:22.991 T:3716150000    INFO: ffmpeg[DD7FF2F0]:     Stream #0:4[0x908](eng): Subtitle: dvb_teletext ([6][0][0][0] / 0x0006)

That said, I only record from UK HD channels so not sure whether other codecs would be equally affected.

As mentioned, increasing the analyze duration (per #15612) would also resolve the problem, as then the correct video frame rate is calculated by ffmpeg, and this is passed along as the hint (rather than 90,000fps).

@jahutchi jahutchi force-pushed the jahutchi:aml-validate-hints branch from 03589c1 to 1124c04 Mar 7, 2019

@kszaq

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Is there any callback for video decoder - in this case DVDVideoCodecAmlogic - that can be used to notify of frame rate change?

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

For me the correct way to do is call from renderer -> amlcodec.
If you look at the log, you see that renderer is reconfigured on players frame change

IIRC you already have the AMLCodec instance as pointer in your videoBuffer

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

For me the correct way to do is call from renderer -> amlcodec.

@peak3d i've been working on this and have got it working... :-)

However I'm struggling with interlaced content.

For example SD 570i 50Hz live tv channels which the renderer is treating as 50fps rather than 25fps. This is leading to stuttered playback as video_rate gets set to 1920 rather than 3840 with my new proposal.

I was thinking i could just always half the framerate for interlaced content before making the call from the renderer to AMLCodec.- e.g
if (picture.iFlags & DVP_FLAG_INTERLACED)...

My question is whether interlaced content is always reported by the renderer as having double framerate?

UPDATE: actually it looks like picture.iFlags is always set to 0 for aml, so this may be a non-starter

@jahutchi jahutchi force-pushed the jahutchi:aml-validate-hints branch 2 times, most recently from 6239024 to 01ca7fd Mar 12, 2019

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

If it's not (currently) possible to correctly calculate the fps for interlaced content in the Renderer (AML), and we're not getting the correct rate from ffmpeg, then I guess we're back to original idea of using a fallback :-(

Looking at this more closely, VideoPlayerVideo itself uses a fallback of 25.0fps when the framerate > 120, which happens after the hints have been set:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/VideoPlayerVideo.cpp#L179

So maybe we just follow the same logic in AMLCodec.cpp? I've adjusted the PR accordingly for consideration.

I'm finding that playback is OK as long as video_rate is sufficiently high (with video_rate being inversely proportional to fps). However, not too high; I tried setting video_rate to 96000 (UNIT_FREQ) as an experiment and found strange things happening when seeking. So 25.0fps (video_rate: 3840) should be a reasonable default.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

VideoPlayer calculates the correct FPS (50 for interlaced 25fps, because we render progressive).
This correct fps can be passed from renderer to AML codec.

Edit:The default impl. you mentioned in your link is only for startup, but then the differences between frame times are calculated and from this is build the correct FPS

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

VideoPlayer calculates the correct FPS (50 for interlaced 25fps, because we render progressive).
This correct fps can be passed from renderer to AML codec.

Yeah, but it screws up playback (causes stuttering) if 50fps is passed along to amlcodec for 25fps interlaced content...as mentioned here....

For example SD 570i 50Hz live tv channels which the renderer is treating as 50fps rather than 25fps. This is leading to stuttered playback as video_rate gets set to 1920 rather than 3840 with my new proposal.

When ffmpeg does detect the framerate for these streams within the 0.5secs then it detects them as 25fps which is why it works.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

For me this is all very, very confusing. Setting 50 instead 25 stutters, but setting 60 (initial way) doesn't? Does this really makes any sense? 50fps is totally correct from kodi POV, because we render 50 frames per second. Even AML decoder produces 50 frames per second, and not 25. I still have the feeling that some things are theoretical nature. @kszaq can you pls. support here?

Edit: Setting 25 could be fine for you living in a country where most live content is 25/50. But what is with ppl. from U.s. for example? They most likely have the same issue but with 29.97 content.
This is the reason I really mislike the hardcoded 25 approach here

@kszaq

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@peak3d Sorry, I'm AFK for the next week.

Perhaps @afl1 can help, he created patches for the issue for CE.

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

For me this is all very, very confusing. Setting 50 instead 25 stutters, but setting 60 (initial way) doesn't?

I can see why this is confusing.... I'll do my best to explain....

My problematic test clips (reported at 90,000fps) were HD recordings: 1080i 50Hz

The only reason these were playing correctly when using my monitor refresh rate (50fps) was thanks to this existing piece of code in the video_rate calculations:

// check for 1920x1080, interlaced, 25 fps
// incorrectly reported as 50 fps (yes, video_rate == 1920)
if (hints.width == 1920 && am_private->video_rate == 1920)
{
CLog::Log(LOGDEBUG, "CAMLCodec::OpenDecoder video_rate exception");
am_private->video_rate = 0.5 + (float)UNIT_FREQ * 1001 / 25000;
}

On testing again at 60fps I do see stuttering - just not as bad as when the 90,000fps rate was used (where video playback basically freezes for several seconds at a time).

I agree that the best way to resolve this is by using the fps rate calculated by the Renderer, but it seems this must be halved for interlaced content (which doubles the video_rate), otherwise it screws up playback of interlaced content (only HD channels/recordings are OK, thanks to the piece of code above).

Here is my attempt so far to track the renderer framerate changes, and adjust the AML video_rate accordingly: jahutchi@ac5f1d8

However, I've not yet found a way to detect whether the content is interlaced:

  if (picture.videoBuffer)
  {
    CLog::Log(LOGDEBUG, "CRendererAML::Configure: picture.iFlags=%d", picture.iFlags);

    //Adjustment for interlaced content
    if (picture.iFlags & DVP_FLAG_INTERLACED)
    {
      fps = fps / 2;
      CLog::Log(LOGDEBUG, "CRendererAML::Configure: interlaced detected, halving fps to: %f", fps);
    }
    CAMLVideoBuffer *amli = dynamic_cast<CAMLVideoBuffer *>(picture.videoBuffer);
    amli->m_amlCodec->SetVideoRateFps(fps);
  }

picture.iFlags is always set to 0 :-(

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@jahutchi

1.) It goes into the right direction now, great :-)
2.) VideoBuffer is what you have created in Decoder: https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDCodecs/Video/AMLCodec.cpp#L2117
If you want an interlaced flag transported, you should set it there, but t.b.h. I wouldn't make thei /2 calculation in renderer. You should do it in Decoder. Then you don't have to pass the flag, but still must investigate from where you get the information if interlaced or progressive. I would bet that aml api provides something for it, if not ffmpeg (hints) ??

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@peak3d

The aml api actually provides the decoder fps rate in codec_get_vdec_state.

The great news is that this fps rate contains the exact value we need (e.g. 25fps for interlaced 50i content). Note: this gives us an integer rather than a float (e.g. for 23 for 23.98 content). Though as this is the rate reported by the decoder, I guess we go with it for our video_rate calculation.

  struct vdec_status vs;
  m_dll->codec_get_vdec_state(&am_private->vcodec, &vs);

  CLog::Log(LOGDEBUG, "CAMLCodec::Decode: decoder status: w:%d h:%d f:%d e:%d s:%d", vs.width, vs.height, vs.fps, vs.error_count, vs.status);

I added this code to be triggered on every call to GetPicture and watched the logfile.

vs.fps starts out at -1, but gets populated very quickly; in my tests, within < 0.1 seconds after CAMLCodec::OpenDecoder, so well before the first reconfigure call from the renderer (when framerate has been calculated).

I should easily be able to adjust my PR to use this.

I have a few different solutions in mind, but not sure which one to go with:

  1. Call on renderer re-configure and set correct video_rate only when invalid initial hints were received from ffmpeg (resolve original issue - e.g. clips reported as 90,000fps).
  2. Call on renderer re-configure and always use this to (re)adjust the current video_rate (adapt to any frame-rate changes).
  3. Call more frequently (on every frame?) to assess whether the rate has changed, and if so then adjust video_rate accordingly (possibly too expensive, but would adapt to framerate changes quicker).

Would be interested to hear your thoughts on this before deciding which path to take.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

@jahutchi is the frame rate reported from aml at this code line already fine:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDCodecs/Video/AMLCodec.cpp#L694 ?

If not, go the simple way and request it on every GetPicture call, and if it has changed regarding the previous one, set it.

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

@jahutchi is the frame rate reported from aml at this code line already fine:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDCodecs/Video/AMLCodec.cpp#L694 ?

If not, go the simple way and request it on every GetPicture call, and if it changed regarding the previous one, set it.

Looks like it can take a few write_av_packet/GetPicture calls before a value is assigned to the fps reported by decoder api...

2019-03-15 21:25:28.891 T:3568288496  NOTICE: CAMLCodec::OpenDecoder - using V4L2 pts format: 64Bit
2019-03-15 21:25:28.892 T:3568288496   DEBUG: write_av_packet: fps was 90000 now -1
2019-03-15 21:25:28.892 T:3568288496   DEBUG: write_av_packet: decoder fps change detected: from 90000 to -1 fps, video_rate adjusted to 0 to compensate
2019-03-15 21:25:28.892 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.892 T:3568288496   DEBUG: write_av_packet: fps was -1 now -1
2019-03-15 21:25:28.892 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.893 T:3568288496   DEBUG: write_av_packet: fps was -1 now -1
2019-03-15 21:25:28.893 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.893 T:3568288496   DEBUG: write_av_packet: fps was -1 now -1
2019-03-15 21:25:28.893 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.894 T:3568288496   DEBUG: write_av_packet: fps was -1 now -1
2019-03-15 21:25:28.894 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.894 T:3568288496   DEBUG: write_av_packet: fps was -1 now -1
2019-03-15 21:25:28.894 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.894 T:3568288496   DEBUG: write_av_packet: fps was -1 now -1
2019-03-15 21:25:28.894 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.894 T:3568288496   DEBUG: write_av_packet: fps was -1 now -1
2019-03-15 21:25:28.895 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.895 T:3568288496   DEBUG: write_av_packet: fps was -1 now -1
2019-03-15 21:25:28.897 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=0 video_fps=-1
2019-03-15 21:25:28.897 T:3568288496   DEBUG: write_av_packet: fps was -1 now 25
2019-03-15 21:25:28.897 T:3568288496   DEBUG: write_av_packet: decoder fps change detected: from -1 to 25 fps, video_rate adjusted to 3840 to compensate
2019-03-15 21:25:28.897 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=3840 video_fps=25
2019-03-15 21:25:28.908 T:3568288496   DEBUG: write_av_packet: fps was 25 now 25
2019-03-15 21:25:28.908 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=3840 video_fps=25
2019-03-15 21:25:28.908 T:3568288496   DEBUG: write_av_packet: fps was 25 now 25
2019-03-15 21:25:28.908 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=3840 video_fps=25
2019-03-15 21:25:28.923 T:3568288496   DEBUG: CAMLCodec::GetPicture: video_rate=3840 video_fps=25
2019-03-15 21:25:28.928 T:4072861696   DEBUG: CAMLCodec::SetVideoRect:display(0,0,1920,1080)
2019-03-15 21:25:28.928 T:4072861696   DEBUG: CAMLCodec::SetVideoRect:gui(0,0,1920,1080)
2019-03-15 21:25:28.928 T:4072861696   DEBUG: CAMLCodec::SetVideoRect:m_dst_rect(0,0,1920,1080)
2019-03-15 21:25:28.928 T:4072861696   DEBUG: CAMLCodec::SetVideoRect:dst_rect(0,0,1920,1080)
2019-03-15 21:25:28.928 T:4072861696   DEBUG: CAMLCodec::SetVideoRect:m_guiStereoMode(0)

Looks like SetVideoRect would be more suitable....which seems to already do various checks of a similar nature. Decoder fps always seems to have a value when SetVideoRect is first called...

@jahutchi jahutchi force-pushed the jahutchi:aml-validate-hints branch from 01ca7fd to 3f45e8c Mar 18, 2019

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@peak3d I've come up with a potential solution (PR updated) so would value your feedback.

So far, I've tested this against the several problematic tvheadend recordings I've kept hold of (where ffmpeg does not initially correctly calculate the frame rate), and it does the trick.

I've also tested using a 23.976fps clip, and that was fine too.

I intend to test this further using some sample files, with various other framerates, to ensure those still playback correctly.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

I will look over it today, thx so for for your commitment!

@jahutchi jahutchi force-pushed the jahutchi:aml-validate-hints branch from 3f45e8c to 47d6eec Mar 18, 2019

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

noticed a typo in my commit message, so just corrected that

@kszaq

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@jahutchi FWIW your patch does not break the few problematic samples I have, so it looks OK. 👍

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I intend to test this further using some sample files, with various other framerates, to ensure those still playback correctly.

I've had my box running with this patch over the past few days, and have not noticed any problems.

I also took the time to run through many of the sample clips at various frame rates from the official samples page. In each case, comparing the results before and after the patch. I haven't seen any regressions (also running with the PlayerDebug info on-screen, to watch for any dropped or skipped frames). Admittedly, not all the clips playback correctly (e.g. VC-1 1080i/29.970 MKV w/DTS-HD MA 5.1 audio), but those same clips don't playback correctly without this patch in place either.

Furthermore, all my problematic tvheadend recordings now playback perfectly (issue I was trying to resolve in the first place).

@kszaq

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@jahutchi VC-1 playback has always been problematic on Amlogic.

While I may not have covered all possible cases, I can confirm that no regressions were observed on my "problematic samples" library and this can go in. 👍

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@peak3d good to go?

struct vdec_status vs;
m_dll->codec_get_vdec_state(&am_private->vcodec, &vs);
CLog::Log(LOGDEBUG, LOGVIDEO, "CAMLCodec::GetDecoderVideoRate: decoder status: w:%d h:%d f:%d e:%d s:%d", vs.width, vs.height, vs.fps, vs.error_count, vs.status);
return (int)(0.5 + ((float)UNIT_FREQ / (float)vs.fps));

This comment has been minimized.

Copy link
@Rechi

Rechi Mar 25, 2019

Member

use C++ style casts

This comment has been minimized.

Copy link
@jahutchi

jahutchi Mar 25, 2019

Author Contributor

Done

This comment has been minimized.

Copy link
@jahutchi

jahutchi Mar 26, 2019

Author Contributor

The change I made yesterday to use C++ style casts has caused an issue where video can freeze for several seconds following seek and pause/play operations.

I'm just working on a solution to this and will submit a further PR to fix things up once I've had time to test it properly.

@jahutchi jahutchi force-pushed the jahutchi:aml-validate-hints branch from 47d6eec to 3767ff3 Mar 25, 2019

@jahutchi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Also noticed a "comparison between signed and unsigned integer expressions [-Wsign-compare]" compiler warning here:
if (video_rate > 0 && video_rate != am_private->video_rate)

So have corrected that too (one less compiler warning to worry about).

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

thx @jahutchi !

jenkins build this please

@peak3d peak3d merged commit 815aded into xbmc:master Mar 25, 2019

1 check passed

default You're awesome. Have a cookie
Details

@Rechi Rechi added this to the Leia 18.2-rc1 milestone Mar 25, 2019

@jahutchi jahutchi referenced this pull request Mar 29, 2019

Merged

[AMLCodec] dont poll decoder rate when seeking #15835

4 of 13 tasks complete

@jahutchi jahutchi deleted the jahutchi:aml-validate-hints branch Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.