when using soemthing like:
mencoder -chapter 1-1 -ovc raw -noskip -field-dominance -1 -vf scale,format=i420,yadif=0,scale,format=i420 -forcedsubsonly -nosub -nosound -mc 0 -lavdopts threads=8 -really-quiet -of rawvideo -o - -dvd-device "G:\TestClips&Co\discs\DVDs\TestDVD" dvd://1 | x264 --preset veryfast --crf 18.00 --profile high --level 4.1 --ref 3 --direct auto --b-adapt 0 --sync-lookahead 48 --qcomp 0.50 --rc-lookahead 40 --qpmax 51 --partitions i4x4,p8x8,b8x8 --no-fast-pskip --subme 5 --aq-mode 0 --vbv-maxrate 62500 --vbv-bufsize 78125 --sar 64:45 --non-deterministic --range tv --colormatrix bt709 --demuxer raw --input-res 720x576 --input-csp i420 --input-range tv --input-depth 8 --fps 25/1 --output-depth 8 --output "G:\Output\test.264" -
it creates broken output, since dvdreads output:
libdvdread: Could not open input:
libdvdread: Can't open G:\TestClips&Co\discs\DVDs\TestDVD for reading
libdvdread: Device G:\TestClips&Co\discs\DVDs\TestDVD inaccessible, CSS authentication not available.
seems to end up in the raw pipe and thus breaks the video send to x264. This didn't happen with older builds, but happens with newer ones. (I build mencoder/mplayer with https://github.com/m-ab-s/media-autobuild_suite.)
Attribute gcc_struct is only defined for x86 architecture and should not be used also on arm or aarch64.
Rebased and runtime tested again with my (quick and dirty) PoC branch
Thanks for the merge @hpi ! And separate thanks for the reviewing style - such attitude to contributors is a pleasure!
Petri Hintukainen (ba2227bb) at 02 Nov 08:59
Add missing stream callback functionality
Investigating a Debian bug report https://bugs.debian.org1020452 I decided to test every codepath of DVD playback: VIDEO_TS/VIDEO_TS.IFO
, videotest.iso
and http://x.x.x.x/videotest.iso
. I did not install libdvdcss as it is not present in Debian repositories for legal concerns. I found that the code binding stream callbacks to libdvdread was moved long ago when libdvdcss was split.
This bug largely went unnoticed because most people likely have followed the steps to compile libdvdcss as mentioned on (outdated) Debian wiki page.
I restored the code and tested it by playing videotest iso. Without this patch, video playback failed, with it it played.
Steps to reproduce:
apt-get update && apt-get install kodi kodi-eventclients-kodi-send
/tmp/videotest.iso
: kodi-send --action='PlayMedia(/tmp/videotest.iso)'
@jbk is it possible to merge this before we get to videolan/libdvdread#40 ? We need that PRs upstream because of Kodi bugs.
If you want I will provide a docker-compose file to rapidly deploy the test environment for you!
I'm sorry I didn't notice this before... Should this be 'if (ret < 0)' ? file_seek() returns < 0 on error and new block position on success.
Other than this the patch looks very good.
@hpi Can you please look one more time as I have resolved all the spots? If you have no objections afterwards, let's mark it as mergeable.
Once it is merged, we will rebase !44 (merged)
Yup! Fixed!
Done! I think moving the blocks_read at the end of the loop is fine because if ret == 0
then no bytes were read at all and we just return the count of previously read blocks.
Good! Removed the redundant checks in file_seek / file_read!
I think it is redundant here, file_open() already checks that either fd >= 0 or stream_cb != NULL. But, yes, it is still good practice to initialize it to -1 in dvd_input_New().
Does this leak dev ? (this check was earlier before alloc).
I think this may cause rounding errors if multiple read returns partial block(s). It would be safer to use total bytes here ( bytes += ret; blocks_read = bytes / DVD_VIDEO_LB_LEN ), or update blocks_read later / after loop.
@hpi can you please look again on this MR? I implemented the dev->ipos
emulation to eliminate SEEK_CUR
part.
I tested on various sources and playback works.
yeap it would be nice to advance this. I don't think it'll be part of the upcoming kodi release (we're reaching Beta/RC) but if it gets in (and lib versions are bumped) it'll give enough time for distros to sync in time for unwrapping the DLL in the next release with no perceivable impact for users.
I think interfaces are OK. It's bit confusing at css side, as there are two different pf_seek's with different signatures. dvdcss->pf_seek takes 'int block_number', while dvdcss->p_stream_cb->pf_seek takes 'uint64_t byte_offset'.
See
https://code.videolan.org/videolan/libdvdcss/-/blob/master/src/dvdcss/dvdcss.h#L49
I dug at the libdvdcss code and I noticed that pf_seek has int pos
while pf_seek in libdvdread has uint64_t pos
. If it is expected to be unsigned 64-bit integer everywhere, we must fix libdvdcss and emulate SEEK_CUR
with dev->i_pos
. Otherwise we have to fix libdvdread.
What is the correct intention here?
I intentionally added this check (and the correspondent one in dvd_input_New) to ensure one DVD input encapsulates one file input. Is it a redundant safety measure?
@jbk ping ?