[vlc-devel] [PATCH] Phosphor deinterlacer, reworked++

[Juha Jeronen]

Hi all,

Any comments on the latest Phosphor?

As far as I'm concerned, it's functionally final, so I would appreciate
a review. If I sound like I'm in a hurry, my apologies - it's just that
this patch is blocking the preparation of the updated traditional IVTC.

The following can be considered known issues:

- The two FIXMEs. As for the input/output pitch mismatch case in
DecayLuma(), in real-world use the present strategy is probably good
enough. As for YV12, I'll need someone knowledgeable to comment on this.

- Default chroma handling mode for 4:2:0 input. As I explained in the
post on the 24th (last Thursday), the sane choice is either AltLine (for
telecined input) or Latest (for true interlaced input). So the question
is, which is the more probable use case? For anime fans with NTSC DVDs,
it's definitely telecined, but does anyone know how is the user
demographic of VLC? And which part of the demographic can we consider as
the most likely to use Phosphor?

- The help text for 4:2:0 chroma handling is too long. The
"film"/"video" input difference should be briefly mentioned, and the
correct algorithm suggested for each (not merely "good", but "correct").
The user labels for the modes should be made descriptive in this regard.
I'll fix this.

- Perhaps the "Off (no light decay)" label for the old field dimmer
would be better as simply "Off". I'll change this if there are no
objections.

- There is a bug in DecayLuma() related to handling video widths not
divisible by 8. For the last pixels, the remove_high bitmask should be
uint8_t (just one pixel), not uint64_t (as it is for the part that *is*
divisible by 8). If the compiler truncates it, it'll work, but
conceptually it's wrong. I'll fix this.

- Also in DecayLuma(), the loop handling the last pixels in each line
could be a separate loop, since most (if not all?) video widths *are*
divisible by 8. Looping through the video height a second time in the
rare cases that there is a width remainder, should be better on average
than re-checking the remainder on each line.

- I'm not satisfied in the duplication between compose_chroma_t and
phosphor_chroma_t. But since they're conceptually different, I'm not
sure if it's reasonable to do anything to this. I'm open for input, but
if there are no objections, I'll leave it as-is.

- The loop for PHOSPHOR_WORKING_AREA_SIZE makes no sense for one entry.
It's included because Phosphate/IVTC2x needs two slots. Maybe it should
be simplified in this patch, and then included in Phosphate/IVTC2x if
that patch is deemed useful.


 -J


On 02/24/11 01:34, Juha Jeronen wrote:
> Hi all,
>
> Attached is the updated Phosphor deinterlacer, meant for review. Please
> comment :)
>
> Changelog:
>
> - 4:2:0 chroma handling mode is now a runtime option. Find it at (All
> settings > Video > Filters > Deinterlace).
>
> - Old field dimmer strength is now also configurable in the settings.
> Default is "low", which is lighter than it used to be.
>
> - Removed an assert in DecayLuma() that shouldn't have been there (video
> pitch % 8 == 0). Instead, if there are any pixels left over, they are
> handled separately.
>
> - Added myself as author for the Phosphor mode.
>
>
>  -J
>