[Libva] [PATCH 0/4] Add support for MVC High profile

[Zhao, Yakui]

On Mon, 2014-06-02 at 23:03 -0600, Gwenole Beauchesne wrote:
> 2014-06-03 5:10 GMT+02:00 Zhao, Yakui <yakui.zhao at intel.com>:
> > On Mon, 2014-06-02 at 11:58 -0600, Gwenole Beauchesne wrote:
> >> Hi,
> >>
> >> This patch series adds support for H.264 Multiview High profile on
> >> Haswell and newer generations.
> >>
> >> Support for H.264 Stereo High profile is readily possible with the
> >> earlier patches for all generations starting from Sandybridge.
> >> Theoritically, earlier generations (e.g. Ironlake) might also be
> >> supported but I have not tested them. It's better to stick to hardware
> >> that support the MFX engine for fixed-function decoding.
> >>
> >> The interesting patch is only the last one. The first three patches
> >> are only convenience to further factor out the frame store logic. More
> >> re-factoring is possible and needed. This is not going to be scalable
> >> to future generations otherwise.
> >
> > This patch set looks clearer than the previous version. But there still
> > exist some problems.
> 
> It's not a replacement for the previous version, it's additions.
> http://lists.freedesktop.org/archives/libva/2014-May/002133.html
> 
> The first series is a pre-requisite in view to fixing/supporting
> Stereo High profiles if you prefer. This second series is for
> supporting Multiview High profiles, and optimizing for AVC profiles
> too on Haswell and newer. But anyhow, this is needed for AVC cases
> too. This just happens to be useful and needed to MVC cases as well.
> 

Sorry that I don't notice this dependency. It will be better that you
can mention the dependent patch set explicitly in the patch description.

> >    1. The patch can't be applied on the latest staging or master branch.
> > And it is difficult to review it. Will you please assure that the patch
> > set is rebased on the latest staging branch ?
> >    2. Patch 03 uses some structures that is not defined.(Maybe this is
> > caused by that it is not rebased on the latest staging branch.)
> 
> The patch series is against master and the above-mentioned patches.
> 
> >    3. If only the last patch is to fix the problem, can it be generated
> > without the previous too many re-factoring patch? Otherwise it will have
> > to spend a lot of time on reviewing the re-factoring patch instead of
> > feature supporting or bug fixing. At the same time it will help to
> > reduce the possible conflict when chery-picking it from staging to
> > master.
> 
> The re-factoring series is mechanical. If you understand the logics,
> it's not hard to review, or propose alternatives. So, what issues do
> you have with the re-factoring? Do you like living with various code
> duplicates? If not, what is the timeline for getting things factored
> out? If there is none, then I'd suggest reviewing the re-factoring
> patches too.

I also don't like such duplicated code. My concern is that the feature
or bug fixing takes precedence if the bug fixing or feature enhancement
has no dependency on refactor. If we have more time, it will be great
that we can do some refactoring. 

> 
> Note: I will try to get this series to work with only patch3. If this
> causes me to duplicate code or makes it look too ugly, I will stop and
> rather request you to do the refactoring instead.
> 
> Thanks,
> Gwenole.