Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Deinterlacer in Decklink-Sources optional #154

Closed
MaZderMind opened this issue Sep 5, 2017 · 11 comments · Fixed by #173
Closed

Make Deinterlacer in Decklink-Sources optional #154

MaZderMind opened this issue Sep 5, 2017 · 11 comments · Fixed by #173

Comments

@MaZderMind
Copy link
Contributor

MaZderMind commented Sep 5, 2017

Moved from #137, originally reported by @a-tze

As written in the cm voc/cm#3 issue, I cannot recommend to statically enable deinterlacing in the decklinksources. > This should also be configurable for two reasons:

  • CPU load, we know it's an issue
  • Maybe the user wants to operate in an interlaced "system mode", e.g. tunnel interlaced content.
    Example use case: voctomix output does not end up in the IT continent of the video world, but is configured to do a playout on SDI, and for whatever reason that output has to be/stay interlaced.
@MaZderMind
Copy link
Contributor Author

@a-tze I tend to accept that there are reasons to not deinterlace in the sources (although FrOSCon and parts of SHA already were produced by this Code), but…

CPU is not a valid argument, because we have to deinterlace each source anyway for Tear-Free viewing in the GUI, so actually all Sources and the Mix are deinterlaced anyway, the Mix actually twice (once for the GUI and once for the Stream).

So the only real reason is flexibility.

In order to maximize flexibility (voctomix is already kind of a lego kit) I'd suggest to

  1. add a [source.*] deinterlace=yes/no/auto option, which turns the yadif on or of with auto being the default value
  2. add support for [source.*] interpret-as-progressive=true (same syntax as proposed in Problem with newer ffmpegs (interlace-mode) #137 for TCP sources) which would add a capssetter as described in Problem with newer ffmpegs (interlace-mode) #137

@MaZderMind MaZderMind modified the milestones: 1.1 release, 1.0 release Sep 5, 2017
@a-tze
Copy link
Contributor

a-tze commented Oct 26, 2017

@MaZderMind I like those proposals, but what about merging these two, e.g. having a fourth option like [source.*] deinterlace=flag

I think interpret-as-progressive is not independet from deinterlace, so it should be combined to avoid meaningless combinations. The flag setting would introduce the capssetter while the other 3 values add and configure a yadif filter.

@MaZderMind
Copy link
Contributor Author

MaZderMind commented Oct 27, 2017

The mapping would then be, if I'm not mistaken, like

deinterlace=yes – add yadif (which produces progressive output)
deinterlace=no – do nothing (requires progressive input)
deinterlace=auto, default – ~~~detect if the input is~~~ accept what the source' metadata say about it's interlaced-/progressiveness and add an yadif if desired
deinterlace=assume-progressive – just tag the input as progressive

@Florob
Copy link
Contributor

Florob commented Oct 27, 2017

How would you implement the yes vs. auto options?
In particular adding yadif is no guarantee for deinterlacing, it will happily just pass through data that is not tagged as interlaced. So if this is meant to force deinterlacing you'd have to tag data as interlaced, which means you'd possibly want to ask the user for the exact type of interlacing.

@a-tze
Copy link
Contributor

a-tze commented Oct 27, 2017

@Florob this is an option of yadif: interlace only if it looks like it is required (== auto) or force deinterlacing (==yes). But I dont know if this option can be used in gstreamer.

If its possible to force yadif in gstreamer to work I'd agree with the last comment from @MaZderMind with those 4 options. They should solve the problem for our use case and make it generally more usable for everyone.

@MaZderMind
Copy link
Contributor Author

MaZderMind commented Oct 30, 2017

@a-tze @Florob that's not what I intended to do with auto. Let me explain:
Currently the internal Decklink-Sources use the configured mode to decide if a deinterlace should be added, so if you configure 1080i50 it automatically adds one in the source. This behavior would be the "auto"-mode and thus keep the default.

For tcp-sources no automatic deinterlacer is currently added (the interlace flag after the mkv-demuxer can't be trusted anyway, see bug above); it just assumes the input to be progressive. auto-mode would also keep this behavior.

@a-tze
Copy link
Contributor

a-tze commented Oct 31, 2017

Thanks for clarification, this also makes sense. So "auto" should not use the word "detect", as it basically just makes an educated guess and would miss e.g. treating pSF-in-i50 as progressive input - which is one of the reasons to introduce such an option. So I'm totally fine with this.

@MaZderMind
Copy link
Contributor Author

Actually it seems decklinkavsource.py does always add the yadif. Also, there is no documentation on the gstreamer yadif element and reading tough its source does not give me confidence that it actually does not work when the input is marked as progressive. For this reason I would ditch the auto-Option and instead make no the save default.

@MaZderMind
Copy link
Contributor Author

@Florob do you remember the Reason you added a videoscale and videorate element in the DeckLinkAVSource? It seems problematic to me to have a unconfigurable and probably undesired scaler and rate-changer there.

@Florob
Copy link
Contributor

Florob commented Oct 31, 2017

@MaZderMind They go from the mode configured for the deckink card to the resolution/rate we work with internally. E.g. without the videorate element we can't source from a Tronsmart/Zidoo at 24fps, while internally using 25fps. I agree that ideally no rate or resolution change happens, but the decklink cards being as picky as they are I prefer to have the freedom to change their mode and have the DeckLinkAVSource deal with necessary conversions.

@MaZderMind
Copy link
Contributor Author

fixed in #168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants