[imx] Move deinterlacing into render thread and use gui settings #29
Conversation
|
This is primarily for discussion. We don't need to merge it soon but at least to revise the concept and the current gui settings implementation whether it is correct or needs fundamental changes. To discuss with code is probably easier for devs ;) |
|
I have tested it during the last days and the integration seems to work well. I can turn on/off deinterlacing and xbmc remembers the settings. You just need to know for streams that render too slow that you need turn off deinterlacing (currently only the Sky samples). For end users probably hard to figure out though ... |
|
Cool. I'll test and review this WE. |
|
@smallint Would you mind rebasing on master, so that this can be applied? Thanks |
|
Done. |
| @@ -1638,7 +1640,15 @@ void CLinuxRendererGLES::RenderIMXMAPTexture(int index, int field) | |||
| YUVPLANE &plane = m_buffers[index].fields[field][0]; | |||
| CDVDVideoCodecBuffer* codecinfo = m_buffers[index].codecinfo; | |||
|
|
|||
| if((codecinfo == NULL) || !codecinfo->IsValid()) return; | |||
There was a problem hiding this comment.
Is there an actual reason for doing CDVDVideoCodecIMX::Enter() even if IsValid is false?
There was a problem hiding this comment.
CDVDVideoCodecIMX::Enter() locks the mutex which was done before in IsValid() itself. IsValid() is now not thread safe anymore.
There was a problem hiding this comment.
Mmm... Any reasons for de-thread-safe it ?
There was a problem hiding this comment.
Because the local locking IsValid does not help to prevent segfaults if you call Close at the decoder stage that cleans up all resources. Calling Process after IsValid is dangerous thats why the entire clean up procedure is guarded with Enter/Leave as well as rendering.
|
I pushed the changes. |
|
Following this issue #40, I have just tried this PR and I confirm that I find the de-interlacing on the screen a little worse than before moving the rendering in the rendering thread. On the other hand it greatly improves the experience while browsing the GUI |
|
For me it works quite well but only if I set the deinterlace method to auto. I added for testing both options: low motion (deinterlace) and fast motion (deinterlace half). Unfortunately the names are not freely selectable in the GUI. The latter uses only the current frame while the first needs two frames and is too slow to be used for 1080i on my side. To set IPU_DEINTERLACE_RATE_EN is easy enough but I haven't tried with this PR if it helps. Could you explain what exactly this flag does? |
|
OK, I was not aware deinterlace was associated to low motion. As I said in the issue #40, switching to fast motion improved visibly the behavior from my point of view... |
|
Have you already tried to apply IPU_DEINTERLACE_RATE_EN? I am going to try that tonight. The question is if it has impacts on the performance. If 1080i slows down significantly we would need an additional deinterlacing option which enables IPU_DEINTERLACE_RATE_EN. It is a pitty that xmbc does not allow to assign custom names for deinterlacing modes. The only option is to change the definitions itself according to HAS_IMXVPU. |
|
According to the IPU driver the IPU_DEINTERLACE_RATE_EN flag does nothing if IPU_DEINTERLACE_RATE_FRAME1 is not set correctly. I don't know if that flags needs to be toggled for each input (probably yes). Is there a documentation for the IPU driver and its ioctl calls? I haven't been able to find one. Also LOW_MOTION seems to be implemented wrongly in this PR. At least I am quite sure that I have swapped the input buffers (current - next vs. current - previous). This whole thing needs some more attention ;) |
|
@smallint I know IPU_DEINTERLACE_RATE_EN is at play when the option vdi_rate_double is used for mxc_vout driver and I know it greatly improves the final result. That's why I suggested to investigate its use directly (now that ipu is directly used instead of indirectly through the mxc_vout driver...) For the low motion, as I said, the final result on screen is visibly not good. Here again you are right : It deserves more attention and a fix... |
|
According to mxc_vout.c it is queuing the task twice (IPU -> FB). This means we have to render the image twice and prepare virtual pictures between two real picture to double the rate: 25Hz -> 50Hz. I checked already ipu_device.c and from what I see is that FRAME1 just swaps the top/bottom field. The real work goes in mxc_vout.c to run the IPU task twice for one additional frame. |
|
yes as I have just written : "you are responsible for submitting properly the frames twice for the double rate to work properly." |
|
I was referring to "I guess You don't need an additional ioctl for this" and I think you need one. I am currently on fixing the visual issues with low motion and reworking the buffer management. Lets see if we manage to integrate the double rate properly. |
|
ho, OK sorry... |
|
Now I am confused ;) Isn't ioctl and the ipu task the same? ipu_device.c does not call a second task if FRAME_EN is given. It just adjusts the parameters (e.g. swapping bottom and top fields). As I understand the mxc code, it does not use ioctl at all because it can call the kernel function directly. But we need to use ioctl from user land. Am I totally wrong here? |
|
Sorry I think I have just understood the misunderstanding : I though you were saying that you need a new ioctl code but you only mean that you need an additional call, right ? If so, then we perfectly agree ... ;) |
|
Haha :) Yes, I meant a second call not another ioctl function. No need to hack the kernel. A side question: do you know how to reliably detect if a frame is interlaced? I have a 1080i stream here which is showing interlacing clearly but it reports a field type of VPU_FIELD_NONE. Other SD stream report VPU_FIELD_TOP which is perfect. The sky streams probably do not need deinterlacing but also report VPU_FIELD_TOP, hmmm. |
|
I pushed a fix that solves the picture jumping for me with low motion. Each frame keeps now track of its previous frame and does not rely on the deinterlacers previous frame which can be quite old due to frame drops and such things. It does not help with performance though. Please let me know if that helps with respect to your bad impressions of the previous version. Next thing is to integrate the double rate. Basically it should be straight forward, to hard part is to decide when to activate it or not since the Decoder has not feedback from the GUI. |
|
I have just tested your last push and it is really better with low motion for sure ! Congratulations... |
|
Yep, have it working and it is really smooth, wow! |
|
I pushed the double rate feature. This is very experimental and does only work for SD streams. It is automatically disabled for HD (too slow for me). I noticed a strange flickering pixel line at the top with this mode. But otherwise it looks pretty good. Since I won't find the time to work on it during the weekend I want to share the code with you at least to test or to play with it. 9da0417 is even more important since this enables high motion on request again. |
|
Very nice for double rate ! it was really worthy to enable it... |
|
Sorry I tried and commented before reading your message ;) |
|
Have you tried 9da0417 or 09915d4? 9da0417 fixes the problem that low motion is always used regardless of the GUi deinterlacer mode. 09915d4 implements the double rate feature which really improves the smoothness. Maybe there are still some problems with setting the right state for FRAME1 and for low motion: what is previous and current frame? But it is a proof of concept that it can work but still needs more tweaking. I am off, have a nice weekend! |
|
I have tried both of them, one after the other... So my last comment applies to the double rate implementation in 09915d4 |
|
Stéphan, I am right in my understanding that this vertical oscillation is also present in the original v4l implementation? Is it a bug in the kernel modules? Can we work around it? It seems that is the input buffer is just offset wrongly by one scanline!? |
|
Yes jan, You are right : this vertical oscillation was the same in original v4l implementation |
|
Hi, |
|
Stéphan, I am basically fine with it. Currently the double rate option cannot be disabled for SD streams. I would like to see all 4 options in xbmc (high motion, low motion, high motion + double rate, low motion + double rate) with its corresponding descriptions in the GUI. Is this feasible or would it be accepted by the xbmc devs to add new enumerations and text representations for the IMX6 path to media settings? Another thing is to do the processing itself: I have checked the vpdau implementation and it is reading the media settings directly (not in the renderer) and does the mixing as they call it in a dedicated thread. I would also like to try that path as well to get rid of the quite complex ::UploadTexture implementation. But all this can go into the next run. |
|
I'm not sure it's relevant but a user posted about OTA 1080i being noticeably inferior now http://imx.solid-run.com/forums/viewtopic.php?f=7&t=641&start=110#p6088 to the wolgar.git and however it was deinterlacing/rendering. |
|
As I understand he is complaining that deinterlacing is disabled at all for HD streams and this is the current state of master, yes. He should see the following log entry "IMX: Disable hardware deinterlacing for HD playback" - so nothing new to me. This is exactly what this PR addresses. |
|
I appreciate the improvement but I don't understand how it could worse now. PR 29 is already in the Geexbox he is commenting on. Martin |
|
Worse in what sense? And what version of #29? Since #29 is not merged so far I also included the fix for low motion deinterlacing and added the experimental double rate feature. And even more important for me: what is actually the issue? Is it too slow, no deinterlacing at all although enabled in the GUI? If it renders too slow could you please provide an example? |
|
It would depend on which GeexBox he use the base line has this http://hg.openbricks.org/openbricks/rev/a95a9a4e4fc4 and I kept my experimental builds current to featdeintrender until it stopped merging because of conflicts with fixdts3. Since the i.MX6 is not ready for production use I felt that getting experimental patches into the wild would help you guys with real world testing, but unfortunately I can't control the quality of the reports that you will get from users. Martin |
|
OK, thats the last version. Again, "worse" means he does not get deinterlacing at all? I always checked your 1080i example (the commercial) and I did it 5 minutes ago after rebasing this PR. I can clearly see interlacing when I disable deinterlacing and I see a smooth picture with deinterlacing set to auto. My advice is to check the GUI deinterlacing settings (I think the default is off) and set them to auto. |
|
Thanks for the good suggestion I will enable Auto, with the next master build and whatever it includes. (still not clear if xbmc-imx6.git is continuing with xbmc 14 or forking with Gotham etc) Martin |
|
@smallint : thanks a lot for your feedback : I propose to open a new PR with all stuff except double rate and we merge it right now... |
|
@wolfgar I removed the double rate stuff in this PR since its name is quite descriptive right now ;) Lets open a new PR for the double rate, OK? |
|
that's perfect to me. |
[imx] Move deinterlacing into render thread and use gui settings
This moves deinterlacing into the render thread and uses the GUI settings to configure deinterlacing during runtime.