hdl-util / hdmi

Send video/audio over HDMI on an FPGA
https://purisa.me/blog/hdmi-released/
Other
1.04k stars 111 forks source link

Potential violation of the HDMI 12 pixel minimum control period #43

Closed krisdover closed 10 months ago

krisdover commented 10 months ago

Hi Sameer, thanks so much for this amazing project. I've been porting bits of it into someone else's input lag tester project which I'm currently modifying to output proper HDMI (instead of DVI). I'm really just after the AVI InfoFrame functionality so I can force the HDMI sink I want to test (some FPV goggles) into RGB mode since it unfortunately defaults to expect the YCbCr video format. I've made good progress but I've noticed a discrepancy in the code which doesn't seem to match the HDMI Spec when it comes to the timing for the preamble:

Section 5.2.5.3 - Island Placement and Duration says:

All TMDS Control Periods shall be at least tS,min (12) characters (pixels) long.

Since the preamble is sent using a control period, it should always be preceded by at least a 4 pixel control period as show in the spec's timing diagram:

image

However the code seems to assume that data island preamble starts immediately after the video data period:

https://github.com/hdl-util/hdmi/blob/b5bc3d72d6b7b6a465cd36e2d215881590f6e31e/src/hdmi.sv#L293

Instead, souldn't this be:

data_island_preamble <= num_packets_alongside > 0 && cx >= screen_width + 4 && cx < screen_width + 12;

This of course would have flow effects for the calculations of guard period and data island period.

Anyhow, thanks again for all your hard work and apologies if I'm completely misinterpreting the spec. :)

sameer commented 10 months ago

Hi Kris,

Thanks for the detailed report, you're definitely correct. The data island preamble starts prematurely and runs for 8 pixels, when instead it should be delayed by >=4 to get the control period length of 12 pixels.

I'm surprised this has not come up before, maybe most sinks are a little forgiving on this.

There are a few things that need to change:

sameer commented 10 months ago

If you would be interested in creating a pull request for this, I would be happy to review it 🙂. Otherwise, I'll update the repo at some point.

Let me know how you'd like to proceed. Thanks again for finding this and reporting it

sameer commented 10 months ago

Looking through the issues, this could be the root cause of the issue @mopplayer was having in #35

krisdover commented 10 months ago

Looking through the issues, this could be the root cause of the issue @mopplayer was having in #35

https://imgur.com/ArIkfPk

Yes, I was definitely getting the same 2-pixel white bar to the left of the display as reported in issue #35, that is until I fixed the minimum control period bug.