fabled / bmd-tools

Tools to connect and manage Blackmagic Design video equipment with built-in H.264 encoder
MIT License
37 stars 14 forks source link

Native modes and scaler setting #16

Closed max-verem closed 8 years ago

max-verem commented 8 years ago

proposed commits implements native modes for 1080i50 and PAL that helps to get interlaced video.

other addons implement scaler settings which helps to produce 480p streaming modes for some reason (it actually has a limitation, for Progressive PAL scaled resolution width greater then 768 gives strange artifact, but it works)

thanks for that project - you did a great job!

fabled commented 8 years ago

The scaler settings look good, perhaps just remove the extra ()'s not needed in many of the ?: places.

Could you explain a little bit more why/what the PAL/1080i50 native mode changes do / are needed? I'd rather not add extra configuration blocks. But that's an option if there's no other easy why to accomplish needed thing. I'd assume that for 1080i50 it changes the 1088/1080 scaling knob? But what it does with the PAL mode?

max-verem commented 8 years ago

Removing parenthesis will change operations priority and as result wrong result, for example:

printf("%d\n", (1 < 2)?4:5 + 6);
printf("%d\n", ((1 < 2)?4:5) + 6);

will output

4 10

Native mode required if you need to keep original quality of signal for recording or transmitting encoded signal to another point. "Progressive" preset seems enable deinterlacer engine that blend fields of interlaced picture. it is good for web, but for not for broadcaster or production because it /degrade/ picture quality

Another interesting thing is that registers 0x0015a2 and 0x0015a4 seems responsible for output canvas. Even native 1080i50 mode ffprobe says: [...] width=1920 height=1080 coded_width=1920 coded_height=1088 [...] i found that .r1430_l related what height 1080 or 1088 will stream has.

fabled commented 8 years ago

The extra parenthesis is on (ep->foo)?a:b can be written as ep->foo?a:b. The -> operator takes precendence over ?: operator. The other parenthesis are required as you noted.

So basically the 'program_fpga' enables hardware de-interlacer? Could native mode flag then be just tested together with 'program_fpga', 'convert_to_1088' and perhaps 'interlaced'?

max-verem commented 8 years ago

setting program_fpga to 1 on native mode does not produce any data at all... parameter convert_to_1088 should not be used (IMHO) used at all, because for 1080i it returns 1088 rows in all cases and should not be scaled...

max-verem commented 8 years ago

to be more precise, convert_to_1088 should be optional for "progressive" mode, if user notice that original picture padded by black lines he could crop and scale usefull lines to full picture

max-verem commented 8 years ago

i can resubmit new pull request if there are no more comments/regards

fabled commented 8 years ago

No more comments. The scaler thing is good after the removal of the few extra parenthesis. The 'progressive' mode stuff should be special cased via a flag - not duplicating the register values. It might even work out nicely as a single commit that way. But conceptually it's good. Thanks!