Okay, I think I've gotten to the bottom of it. BD+ discs work properly with libmmbd in 1.15.4. That version used shared memory to communicate between libmmbd and makemkvcon. Later versions (checked 1.16.7, 1.17.7, and the current 1.18.4) switched to using pipes for that communication. In the process, they introduced two bugs that impact libbdplus emulation.
Bug 1: High bits of offset lost
This is the bug that causes the garbage after 20 minutes.
Before the change:
Code: Select all
typedef struct _AP_SHMEM
{
uint32_t cmd;
uint8_t start;
uint8_t abort_nomem;
uint8_t exit;
uint8_t pad0[1];
uint32_t pad1[4];
uint64_t args[32]; // <-- 64-bit
utf16_t strbuf[32768];
} ALIGN_PACKED AP_SHMEM;
Code: Select all
const uint8_t* CMMBDApClient::DecryptUnitMMBD(uint32_t NameFlags,uint32_t* ClipInfo,uint64_t FileOffset,const uint8_t* Data,unsigned int Size)
{
m_mem->args[0]=NameFlags;
m_mem->args[1]=*ClipInfo;
m_mem->args[2]=FileOffset; // <-- whole 64-bit offset, one slot
if (Size) memcpy((void*)(m_mem->args+4),Data,Size);
ExecCmd(apCallDecryptUnitMMBD);
After the change:
Code: Select all
typedef struct _AP_SHMEM
{
uint32_t cmd;
uint32_t flags;
uint32_t pad1;
uint32_t pad2;
uint32_t args[32]; // <-- narrowed to 32-bit
uint8_t strbuf[65008];
} AP_SHMEM;
Code: Select all
const uint8_t* CMMBDApClient::DecryptUnitMMBD(uint32_t NameFlags,uint32_t* ClipInfo,uint64_t FileOffset,const uint8_t* Data,unsigned int Size)
{
m_mem->args[0]=NameFlags;
m_mem->args[1]=*ClipInfo;
m_mem->args[2]=(uint32_t)FileOffset; // <-- low bits
m_mem->args[3]=(uint32_t)(FileOffset>>32); // <-- high bits
if (Size) memcpy((void*)(m_mem->strbuf),Data,Size);
ExecCmd(apCallDecryptUnitMMBD,4,Size);
So the client is still sending all 64 bits, but split between two arg slots. It appears, however, that makemkvcon is still only looking at the first offset slot, resulting in the offset effectively getting truncated to 32-bits and thus the video stream to be corrupted past the 4GiB mark.
This is a bug in the binary-only makemkvcon that can only be fixed by the MakeMKV team.
Bug 2: Stale fixups applied to subsequent bytes
If a block doesn't have any fixups to apply, makemkvcon will return a zero-size response and the response buffer will not be overwritten. libmmbd does not currently check the return size, though, so if bdplus_fixup is called on a block with no fixups, but the previous library call was to bdplus_fixup for a block that
did have fixups, libmmbd will read the stale buffer and erroneously apply those same fixups to the new block. This will also corrupt the video in cases where the client application is not strictly interleaving some other call between bdplus_fixup invocations.
The fix for this is entirely on the open-source libmmbd side:
Code: Select all
--- a/makemkvgui/inc/lgpl/aproxy.h
+++ b/makemkvgui/inc/lgpl/aproxy.h
@@ -356,7 +356,7 @@
bool InitMMBD(const char* argp[]);
bool OpenMMBD(const char* Prefix,const char* Locator);
const uint8_t* DiscInfoMMBD(uint32_t *Flags,uint8_t *BusKey,uint8_t *DiscId,uint32_t *MkbVersion,uint32_t* ClipCount);
- const uint8_t* DecryptUnitMMBD(uint32_t NameFlags,uint32_t* ClipInfo,uint64_t FileOffset,const uint8_t* Data,unsigned int Size);
+ const uint8_t* DecryptUnitMMBD(uint32_t NameFlags,uint32_t* ClipInfo,uint64_t FileOffset,const uint8_t* Data,unsigned int Size,unsigned int* RetSize);
};
class CShMemTransport : public CApClient::ITransport
--- a/libmmbd/src/marmmbd.cpp
+++ b/libmmbd/src/marmmbd.cpp
@@ -99,7 +99,7 @@
return p+36;
}
-const uint8_t* CMMBDApClient::DecryptUnitMMBD(uint32_t NameFlags,uint32_t* ClipInfo,uint64_t FileOffset,const uint8_t* Data,unsigned int Size)
+const uint8_t* CMMBDApClient::DecryptUnitMMBD(uint32_t NameFlags,uint32_t* ClipInfo,uint64_t FileOffset,const uint8_t* Data,unsigned int Size,unsigned int* RetSize)
{
m_mem->args[0]=NameFlags;
m_mem->args[1]=*ClipInfo;
@@ -113,6 +113,10 @@
*ClipInfo = (uint32_t)m_mem->args[1];
+ // CPipeTransport::RecvCmd only refreshes strbuf when the reply carries data, so on
+ // a reply of size zero it still holds whatever the previous reply left there.
+ *RetSize = (unsigned int)(m_mem->cmd&0xffff);
+
return (const uint8_t*)(m_mem->strbuf);
}
--- a/libmmbd/src/mmconn.cpp
+++ b/libmmbd/src/mmconn.cpp
@@ -380,7 +380,7 @@
uint32_t clip_info = 0;
uint32_t *p_clip_info = NULL;
const uint8_t* dbuf;
- unsigned int size,ret=0;
+ unsigned int size,ret=0,dbuf_size=0,dbuf_flags;
uint32_t clip_name = name_flags&(0xfffff|MMBD_FLAG_AUTO_CPSID|MMBD_FILE_SSIF);
if ((!m_active) || (!m_disc_flags)) return -2;
@@ -412,9 +412,14 @@
}
- dbuf = m_apc.DecryptUnitMMBD(name_flags,&clip_info,file_offset,buf,size);
+ dbuf = m_apc.DecryptUnitMMBD(name_flags,&clip_info,file_offset,buf,size,&dbuf_size);
if (!dbuf) return -4;
+ // A reply of size zero leaves the previous reply's bytes in the buffer, so the
+ // buffer's flags are stale and not meant for us.
+ dbuf_flags = dbuf_size ? dbuf[0] : 0;
+
if (p_clip_info) {
if (p_clip_info[1]!=clip_info) {
m_last_clip_info[0]=0xffffffff;
@@ -426,7 +431,7 @@
if (0!=(name_flags&MMBD_FLAG_BLOCK_KEY)) {
- if (dbuf[0]&1) {
+ if (dbuf_flags&1) {
memcpy(buf, dbuf+1, 16);
} else {
memset(buf, 0x00, 16);
@@ -438,7 +443,7 @@
if ( ((buf[0]&0xC0)!=0x00) && (0==(name_flags&MMBD_FLAG_BDPLUS_ONLY)) ) {
- if (dbuf[0]&1) {
+ if (dbuf_flags&1) {
if ((m_disc_flags&AP_MMBD_DISC_FLAG_BUSENC)!=0) {
(*m_aes_cbcd)(m_bus_key, iv, buf+16+0*BD_SECTOR_SIZE, BD_SECTOR_SIZE-16);
@@ -456,7 +461,7 @@
}
- if (dbuf[0]&2) {
+ if (dbuf_flags&2) {
const uint8_t* p = dbuf + 17;
unsigned int count = *p++;
for (unsigned int i=0;i<count;i++) {