Opened 13 years ago

Closed 13 years ago

#2312 closed defect (fixed)

New MMX code for YUV420 conversion causes issues

Reported by: pdbailey@… Owned by: Nigel
Priority: minor Milestone: 0.20
Component: mythtv Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

The changes introduced in r11013 cause me an intermittent problem with both live TV and recordings. On changing channels, or on skipping through a recorded programme, the video and audio freezes, and the log fills up with:

"NVP: Timed out waiting for free video buffers"

I was unsure as to the exact cause of this problem, but after backing out the changes in this changeset, I am unable to reproduce the problem, so I'm fairly confident that somehow this change is the cause. There were some posts on the dev mailing list about others on Intel Macs suffering from the exact same symptoms.

I have no crashlog to attach (as the app just freezes), but I will attach a verbose log from the frontend when I get home this evening.

Attachments (4)

playback.log (19.9 KB) - added by pdbailey@… 13 years ago.
Playback log from attempting to view DVD content
osx-packager.pl (36.6 KB) - added by pdbailey@… 13 years ago.
2nd attempt at playback, after patch applied (patch also attached)
playback.2.log (19.3 KB) - added by pdbailey@… 13 years ago.
2nd attempt at playback, after patch applied (patch also attached)
patch.diff (2.6 KB) - added by pdbailey@… 13 years ago.
patch applied (tweaked to try to get it to compile from the one on the mythtv-dev mailing list)

Download all attachments as: .zip

Change History (13)

comment:1 Changed 13 years ago by Isaac Richards

Owner: changed from Isaac Richards to Nigel

comment:2 Changed 13 years ago by Nigel

Status: newassigned

Paul, does the fault occur if you disable "Use Vector-enhanced color space conversion" in the TV Playback settings? This will result in garbage images, but at least it shouldn't change any buffers.

comment:3 Changed 13 years ago by pdbailey@…

Nigel,

My recollection is that it did, but I'll be able to check that in about 45 minutes or so.

comment:4 Changed 13 years ago by pdbailey@…

Ignore my last comment, I was totally wrong. The video plays without that message appearing (but messed up visually, obviously)

comment:5 Changed 13 years ago by Nigel

Milestone: unknown0.20

OK. I will guess that something is overwriting the frame's buffer space and clobbbering videobuffers queue management? Could I get someone to turn the Vector-enhanced setting back on after applying this patch:

% svn diff yuv2rgb.cpp
Index: yuv2rgb.cpp
===================================================================
--- yuv2rgb.cpp (revision 11063)
+++ yuv2rgb.cpp (working copy)
@@ -927,7 +927,7 @@
 #endif
 
 #ifdef MMX
-    return mmx_yuv420_2vuy;
+    //return mmx_yuv420_2vuy;
 #endif
 
     return non_vec_yuv420_2vuy; /* Fallback to C */

which will tell us if QuickTime? is screwing up the frontend's data, or if the MMX routine is. And the output from -v playback would be helpful, too.

comment:6 Changed 13 years ago by pdbailey@…

OK, after applying that patch. I can't reproduce the issue.

Oddly enough, if I run with logging set to -v playback, I can't seem to reproduce it either...

comment:7 Changed 13 years ago by Nigel

1) OK. At least we can easily work around it.
2) Bummer. That seems to indicate more complex problems.
I will try to work out the problem with the routine, but if that doesn't get sorted before 0.20, I will commit something like:

% svn diff yuv2rgb.h yuv2rgb.cpp videoout_quartz.cpp
Index: yuv2rgb.h
===================================================================
--- yuv2rgb.h   (revision 11063)
+++ yuv2rgb.h   (working copy)
@@ -54,6 +54,13 @@
 
 yuv2vuy_fun get_yuv2vuy_conv(void);
 
+extern void non_vec_yuv420_2vuy(uint8_t, uint8_t,
+                                uint8_t, uint8_t,
+                                int, int, int, int, int);
+
+
+// Conversion in the other direction, currently unused
+
 typedef void (* vuy2yuv_fun) (uint8_t * image, uint8_t * py,
                               uint8_t * pu, uint8_t * pv,
                               int h_size, int v_size,
Index: yuv2rgb.cpp
===================================================================
--- yuv2rgb.cpp (revision 11063)
+++ yuv2rgb.cpp (working copy)
@@ -784,10 +784,10 @@
 #endif // MMX
 
 
-static void non_vec_yuv420_2vuy (uint8_t * image, uint8_t * py,
-                                 uint8_t * pu, uint8_t * pv,
-                                 int h_size, int v_size,
-                                 int vuy_stride, int y_stride, int uv_stride)
+void non_vec_yuv420_2vuy (uint8_t * image, uint8_t * py,
+                          uint8_t * pu, uint8_t * pv,
+                          int h_size, int v_size,
+                          int vuy_stride, int y_stride, int uv_stride)
 {
     uint8_t *pi1, *pi2 = image;
     uint8_t *py1, *py2 = py;
Index: videoout_quartz.cpp
===================================================================
--- videoout_quartz.cpp (revision 11063)
+++ videoout_quartz.cpp (working copy)
@@ -1341,7 +1341,13 @@
     if (gContext->GetNumSetting("MacYuvConversion", 1))
         data->yuvConverter = get_yuv2vuy_conv();
     else
+#ifdef WORDS_BIGENDIAN
+        // It is safe on PPC to let QuickTime convert YUV420:
         data->yuvConverter = NULL;
+#else
+        // ... but on Intel it crashes, so we always pre-convert:
+        data->yuvConverter = non_vec_yuv420_2vuy;
+#endif
 
     if (!CreateQuartzBuffers())
     {
@@ -1453,11 +1459,7 @@
     ImageDescription *desc = *data->imgDesc;
 
     desc->idSize = sizeof(ImageDescription);
-#ifdef WORDS_BIGENDIAN
     desc->cType = kYUV420CodecType;
-#else
-    desc->cType = kComponentVideoCodecType;  // Wrong, but prevents Intel crash
-#endif
     desc->version = 1;
     desc->revisionLevel = 0;
     desc->spatialQuality = codecNormalQuality;

Changed 13 years ago by pdbailey@…

Attachment: playback.log added

Playback log from attempting to view DVD content

Changed 13 years ago by pdbailey@…

Attachment: osx-packager.pl added

2nd attempt at playback, after patch applied (patch also attached)

comment:8 Changed 13 years ago by pdbailey@…

Attached my buildscript erroneously

Changed 13 years ago by pdbailey@…

Attachment: playback.2.log added

2nd attempt at playback, after patch applied (patch also attached)

Changed 13 years ago by pdbailey@…

Attachment: patch.diff added

patch applied (tweaked to try to get it to compile from the one on the mythtv-dev mailing list)

comment:9 Changed 13 years ago by Nigel

Resolution: fixed
Status: assignedclosed

(In [11076]) Add emms() that was missing from new MMX code. Closes #2312 Also added a fallback for incompatible source dimensions, but now realise that it will only work horizontally. If we ever get a vertical resolution that is not even, then things will still fail.

Note: See TracTickets for help on using tickets.