vp8 encoder parameter check - do not check for ref frames if no no_re…

[ottingerg]

…f_X flag is set

In VP8 encoding not always all reference frames (last, golden, altref) are provided.
This is indicated with the flags ref_flags.bits.no_ref_last, ref_flags.bits.no_ref_gf
and ref_flags.bits.no_ref_arf in VAEncPictureParameterBufferVP8.
While checking picture parameters, make sure that assert errors concerning invalid
reference frame only occure if no_ref_X is not set.

[ceyusa]

I know this is a pattern in the current code, but it seems contradictory to me.

From wikipedia https://en.wikipedia.org/wiki/Assertion_(software_development)#Comparison_with_error_handling

Assertions are distinct from routine error-handling. Assertions document logically impossible situations and discover programming errors: if the impossible occurs, then something fundamental is clearly wrong with the program. This is distinct from error handling: most error conditions are possible, although some may be extremely unlikely to occur in practice. Using assertions as a general-purpose error handling mechanism is unwise: assertions do not allow for recovery from errors; an assertion failure will normally halt the program's execution abruptly; and assertions are often disabled in production code. Assertions also do not display a user-friendly error message.

Here we have an assert following with typical error management code for the same verification. IMO it would be better to get rid of the asserts in this case.

[ottingerg]

thats a well-considered point.

The only pro-assert argument that comes to my mind is, when the calling functions do not handle the error properly and a hard-exit is needed.

intel_encoder_check_vp8_parameter() is called only a single time by intel_encoder_sanity_check_input() and is proper checked for VA_STATUS_SUCCESS

Also functions like intel_encoder_check_hevc_parameter() do go without asserts.

[ottingerg]

... well another pro-assert argument would be that it was helpful while debugging ...

[sreerenjb]

It is bit unfortunate that we have asserts like this in many places. We should definitely try to avoid it in future. You could provide a separate patch to "avoid asserts".

[ottingerg]

As ceyusa already noted the asserts are followed by a proper error handling - so i think I can just remove them safely. The macro SURFACE(x) relates to the function object_heap_lookup(), which will also return NULL if the object is not allocated - so both asserts can be removed.

Further I am not quite sure how important the proper order of the reference_objects[] slots is - so to play it safe I am following the way of intel_encoder_check_vp9_parameter() and preserve the slot order when handling the reference objects, by filling the current slot with NULL if not used.

[ottingerg]

I just learned that there exists a s_vp8 branch in the https://github.com/laureatian/intel-vaapi-driver/ fork - that is already addressing this Issue. It also includes further necessary patches to get VP8 temporal SVC working.

[ceyusa]

huh?

And how those patches are going to be reviewed and merged?

How's the workflow now for this driver?

[ottingerg]

The above mentioned patches are the prerequisits for VP8 temporal SVC - the buffer Issue was resolved in a manner very similar to my first commit (including the asserts) - But I assume that this patches will be merged, because otherwise VP8 temporal SVC is not working ...

[xhaihao]

There are some quality issues with bitrate control in https://github.com/laureatian/intel-vaapi-driver/, so we won't merge https://github.com/laureatian/intel-vaapi-driver/ before addressing the quality concern.
Actually you needn't https://github.com/laureatian/intel-vaapi-driver/ if you don't need bitrate control.

[sreerenjb]

@xhaihao Do you mean the current git master of intel-vaapi-driver is enough to make the vp8 svc encode work (except the bit rate control)?

[ottingerg]

I just reopened this pull request and added another two lines taken from the laureatian fork, which make sure that there are no wrong copy_buffer_to_golden or copy_buffer_to_alternate occurancies, if refresh_golden / refresh_alternate is set.

[ottingerg]

IMHO this is now the minimal patch in order to get basic vp8 temporal svc working in yamitranscode.

[xhaihao]

@sreerenjb yes although maybe some patches are needed. The patches in the https://github.com/laureatian/intel-vaapi-driver/ are mainly for bitrate control.