Page MenuHome

Fix T54117 Movie clip undistorted - proxy not working
ClosedPublic

Authored by Richard Antalik (ISS) on Jan 17 2019, 2:38 AM.

Diff Detail

Repository
rB Blender

Event Timeline

Not sure how exactly reproduce the error, but from code it feels like you need to pass clip->flag instead of 0 to BKE_movieclip_get_ibuf_flag.

With the current patch it seems you're forcing proxies to be used even for clips which do not have proxies generated, which will make them to be completely disappear.

To reproduce please follow T54117

I haven't checked this with proxy not build, but looking at the code it seems, that in such case original is fetched instead.
Can not test this right now.

Add fallback full size render, if proxy not built

Doing such a manual checks should only be the last resort. I would expect that passing clip->flag BKE_movieclip_get_ibuf_flag() will solve the issue.

The patch forces proxies to be used even for clips which are configured to not use proxies. Which is ok-ish, for until there are outdated proxies left on the disk, and clip has proxies disabled.

Doing such a manual checks should only be the last resort. I would expect that passing clip->flag BKE_movieclip_get_ibuf_flag() will solve the issue.
The patch forces proxies to be used even for clips which are configured to not use proxies. Which is ok-ish, for until there are outdated proxies left on the disk, and clip has proxies disabled.

I can check, if movieclip has proxies enabled.
On the other hand user "explicitly" requested use of proxies in the sequencer and movieclip is "owned" by sequencer in this case.

If I need to check if use of proxies is enabled on movieclip itself, I would propose to do it in movieclip API and add flag to use fallback full-size render in case of disabled / non existing proxies, so other modules don't have to duplicate this set of checks.

You can not use sequencer's proxy system for movie clips. That will conflict with the proxy system of clips.

And you shouldn't be checking clip's flags, but rather just pass them along, or use BKE_movieclip_get_ibuf.

If the issue is that this function will not give you an original frame for the case when proxy file is missing we should consider one of the following:

  • Match the behavior of BKE_movieclip_get_ibuf with the current sequencer behavior (so there is no difference between proxies not generated in sequencer in clip editor). Will probably require some information in the interface saying that proxy load failed, so it is not a quiet failure.
  • Pass a flag to the BKE_movieclip_get_ibuf which will define behavior in case proxies are not generated.
Richard Antalik (ISS) edited the summary of this revision. (Show Details)

This function argument swapping seems rather ugly to me, but I didn't want to modify original data...

Remove unused variable

source/blender/blenkernel/intern/movieclip.c
989

You shouldn't be modifying function signature based on how it deals with temporary variables.

The clip user is not supposed to be changing as far as the caller is concerned, and hence should have const qualifier.

1031–1040

This is weird. The code is only dealing with movie files, ignoring the image sequence case.
The code around need_postprocess also needs clarification. I'm only guessing is that it's because its possible that caller requests undistorted proxy, which might not exist, hence undistortion is needed.

Also guess ideally we should try reading distorted proxy, since that'd be faster to postprocess.

Richard Antalik (ISS) marked 2 inline comments as done.

Haven't realized, that it is possible to add image sequence...

I chose use of recursive call, but function use locks, so code moved after lock release.

Alternatively I can put this code into BKE_movieclip_get_ibuf_flag or create a new intermediate function. Not sure, if that is a OK thing to do.
Otherwise I would have to repeat the code, which I am trying to avoid.

This patch seems fine, but i think extra fix is required on top of this: P917.
That snippet will fix case when proxy is not enabled for the clip, but undistorted clip/proxy is requested. You'll probably want to verify it though.

Other than that, the only issue i've noticed is the commit message needs tweaking (i.e. it contains lines whic hare supposed to be a comment).

P.S. Those flags are becoming a bit tricky, would be nice to clearly separate rendering from postprocessing, but that can happen any other day.

Is P917 patch on top of this diff?

I lost context of this patch so I can't see what you did there.
Will have to look at it applied on the code.

Other than that, the only issue i've noticed is the commit message needs tweaking (i.e. it contains lines which hare supposed to be a comment).

Sorry I don't understand this.

Is P917 patch on top of this diff?

Yep, it is.

Other than that, the only issue i've noticed is the commit message needs tweaking (i.e. it contains lines which hare supposed to be a comment).

Sorry I don't understand this.

Is just when i did arc patch it just did something weird with the commit message. Easy to correct, but need to pay attention before pushing.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 18 2019, 9:12 PM
This revision was automatically updated to reflect the committed changes.