media/gpu: restore some of the reformatting done during migration to media/gpu.

media/gpu: restore some of the reformatting done during migration to media/gpu.

This change is purely cosmetic.

Restore some of the formatting lost after automated reformat during the
move to media/gpu.

BUG=608241
TEST=compile
NOPRESUBMIT=true

Committed: https://crrev.com/d6a739e430d351bf40bf175f1ca15f5f28007d6f
Cr-Commit-Position: refs/heads/master@{#394318}

[Pawel Osciak, 2016-05-12 08:33:33]

posciak@chromium.org changed reviewers:
+ kcwu@chromium.org, markdittmer@chromium.org, mcasas@chromium.org,
owenlin@chromium.org

[Pawel Osciak, 2016-05-12 08:33:42]

The CQ bit was checked by posciak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-12 08:33:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973753003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973753003/1

[owenlin_google, 2016-05-12 09:04:48]

owenlin@google.com changed reviewers:
+ owenlin@google.com

[owenlin_google, 2016-05-12 09:04:48]

lgtm

[kcwu, 2016-05-12 09:42:42]

lgtm

https://codereview.chromium.org/1973753003/diff/1/media/gpu/jpeg_decode_accel...
File media/gpu/jpeg_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1973753003/diff/1/media/gpu/jpeg_decode_accel...
media/gpu/jpeg_decode_accelerator_unittest.cc:257: media::PIXEL_FORMAT_I420,
How about add comments at the end of each line?
It's good to have comments because there are too many parameters for this call.
And it also preserve the layout as side effect.

media::PIXEL_FORMAT_I420,  // format
image_file->visible_size,  // coded size
gfx::Rect(image_file->visible_size),  // visible rectangle
etc.

https://codereview.chromium.org/1973753003/diff/1/media/gpu/v4l2_slice_video_...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1973753003/diff/1/media/gpu/v4l2_slice_video_...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2066: sps->constraint_set0_flag
? V4L2_H264_SPS_CONSTRAINT_SET0_FLAG : 0 |
Will git cl format be happier with parentheses?

I mean
(a?b:c) |
(a?b:c) |
(a?b:c) |
...

[commit-bot: I haz the power, 2016-05-12 09:53:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-12 09:53:21]

Dry run: This issue passed the CQ dry run.

[Mark Dittmer, 2016-05-12 14:13:51]

Comments-on-comments. The "greater-than-1" comment should go next to the spot
where we use a value that's greater-than-1.

https://codereview.chromium.org/1973753003/diff/1/media/gpu/vaapi_jpeg_decode...
File media/gpu/vaapi_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/1973753003/diff/1/media/gpu/vaapi_jpeg_decode...
media/gpu/vaapi_jpeg_decode_accelerator.cc:32: // than 1.
I still don't understand this comment. We have:

VAJDADecoderFailure {
 0: VAAPI_ERROR,
 1: VAJDA_DECODER_FAILURES_MAX,
}

The "MAX" here is NOT greater than 1. Why do we have a comment suggesting that
it is?

https://codereview.chromium.org/1973753003/diff/1/media/gpu/vaapi_jpeg_decode...
media/gpu/vaapi_jpeg_decode_accelerator.cc:38: VAJDA_DECODER_FAILURES_MAX + 1);
Should the above-mentioned comment go here instead? That would make a lot more
sense to me.

https://codereview.chromium.org/1973753003/diff/1/media/gpu/vaapi_video_decod...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1973753003/diff/1/media/gpu/vaapi_video_decod...
media/gpu/vaapi_video_decode_accelerator.cc:38: // than 1.
Ditto.

https://codereview.chromium.org/1973753003/diff/1/media/gpu/vaapi_video_decod...
media/gpu/vaapi_video_decode_accelerator.cc:45: VAVDA_DECODER_FAILURES_MAX + 1);
Ditto.

[Pawel Osciak, 2016-05-13 06:07:22]

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/jpeg_decod...
File media/gpu/jpeg_decode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/jpeg_decod...
media/gpu/jpeg_decode_accelerator_unittest.cc:257: media::PIXEL_FORMAT_I420,
On 2016/05/12 09:42:42, kcwu wrote:
> How about add comments at the end of each line?
> It's good to have comments because there are too many parameters for this
call.
> And it also preserve the layout as side effect.
> 
> media::PIXEL_FORMAT_I420,  // format
> image_file->visible_size,  // coded size
> gfx::Rect(image_file->visible_size),  // visible rectangle
> etc.

Thanks for the suggestion, on the other hand, it may be making the code a bit
verbose, so we agreed to keep as is for now.

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/v4l2_slice...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2066: sps->constraint_set0_flag
? V4L2_H264_SPS_CONSTRAINT_SET0_FLAG : 0 |
On 2016/05/12 09:42:42, kcwu wrote:
> Will git cl format be happier with parentheses?
> 
> I mean
> (a?b:c) |
> (a?b:c) |
> (a?b:c) |
> ...

Indeed! Thank you.

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/vaapi_jpeg...
File media/gpu/vaapi_jpeg_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/vaapi_jpeg...
media/gpu/vaapi_jpeg_decode_accelerator.cc:32: // than 1.
On 2016/05/12 14:13:51, Mark Dittmer wrote:
> I still don't understand this comment. We have:
> 
> VAJDADecoderFailure {
>  0: VAAPI_ERROR,
>  1: VAJDA_DECODER_FAILURES_MAX,
> }
> 
> The "MAX" here is NOT greater than 1. Why do we have a comment suggesting that
> it is?

Please see below.

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/vaapi_jpeg...
media/gpu/vaapi_jpeg_decode_accelerator.cc:38: VAJDA_DECODER_FAILURES_MAX + 1);
On 2016/05/12 14:13:51, Mark Dittmer wrote:
> Should the above-mentioned comment go here instead? That would make a lot more
> sense to me.

Yeah, sorry, the reason is that it's gotten a bit more complicated now after the
migration to media/gpu.

There are two issues actually. First of all, the original issue in UMA, where we
can't have just one bucket in a histogram, please see
https://bugs.chromium.org/p/chromium/issues/detail?id=549054#c6 for explanation.
The second issue is that there is a presubmit hook in media/, which didn't
trigger before we moved this to media/, which requires a particular style for
UMA max values in media/, please see commit message for http://crrev.com/254209.

So with that, normally we'd say:
enum VAJDADecoderFailure {
  VAAPI_ERROR = 0,
  VAJDA_DECODER_FAILURES_MAX = VAAPI_ERROR,
};

UMA_HISTOGRAM_ENUMERATION(..., VAJDA_DECODER_FAILURES_MAX + 1);


However, this hits the first issue. If we instead did:

enum VAJDADecoderFailure {
  VAAPI_ERROR = 0,
  VAJDA_DECODER_FAILURES_MAX = 2,
};

UMA_HISTOGRAM_ENUMERATION(..., VAJDA_DECODER_FAILURES_MAX);

That would have triggered the second check. But I just thought of a different
solution. I'll submit it as a separate CL to keep this as a cosmetic-only
change.

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/vaapi_vide...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/vaapi_vide...
media/gpu/vaapi_video_decode_accelerator.cc:38: // than 1.
On 2016/05/12 14:13:51, Mark Dittmer wrote:
> Ditto.

Acknowledged.

https://chromiumcodereview.appspot.com/1973753003/diff/1/media/gpu/vaapi_vide...
media/gpu/vaapi_video_decode_accelerator.cc:45: VAVDA_DECODER_FAILURES_MAX + 1);
On 2016/05/12 14:13:51, Mark Dittmer wrote:
> Ditto.

Acknowledged.

[Mark Dittmer, 2016-05-13 12:10:11]

Please edit this issue to refer to
https://bugs.chromium.org/p/chromium/issues/detail?id=608241.

[Mark Dittmer, 2016-05-13 15:52:23]

Description was changed from

==========
media/gpu: restore some of the reformatting done during migration to media/gpu.

This change is purely cosmetic.

Restore some of the formatting lost after automated reformat during the
move to media/gpu.

BUG=586386
TEST=compile
NOPRESUBMIT=true
==========

to

==========
media/gpu: restore some of the reformatting done during migration to media/gpu.

This change is purely cosmetic.

Restore some of the formatting lost after automated reformat during the
move to media/gpu.

BUG=608241
TEST=compile
NOPRESUBMIT=true
==========

[Pawel Osciak, 2016-05-16 06:39:41]

The CQ bit was checked by posciak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 06:39:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973753003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973753003/20001

[commit-bot: I haz the power, 2016-05-16 07:50:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-16 07:50:27]

Dry run: This issue passed the CQ dry run.

[Mark Dittmer, 2016-05-17 15:14:52]

On 2016/05/13 06:07:22, Pawel Osciak wrote:
> That would have triggered the second check. But I just thought of a different
> solution. I'll submit it as a separate CL to keep this as a cosmetic-only
> change.

SGTM & LGTM. Please CC me on the new solution.

[Pawel Osciak, 2016-05-18 00:40:31]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-05-18 00:40:32]

The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@google.com,
kcwu@chromium.org
Link to the patchset: https://codereview.chromium.org/1973753003/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-05-18 00:41:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973753003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973753003/20001

[commit-bot: I haz the power, 2016-05-18 03:16:39]

Description was changed from

==========
media/gpu: restore some of the reformatting done during migration to media/gpu.

This change is purely cosmetic.

Restore some of the formatting lost after automated reformat during the
move to media/gpu.

BUG=608241
TEST=compile
NOPRESUBMIT=true
==========

to

==========
media/gpu: restore some of the reformatting done during migration to media/gpu.

This change is purely cosmetic.

Restore some of the formatting lost after automated reformat during the
move to media/gpu.

BUG=608241
TEST=compile
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-05-18 03:16:40]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-18 03:18:13]

Description was changed from

==========
media/gpu: restore some of the reformatting done during migration to media/gpu.

This change is purely cosmetic.

Restore some of the formatting lost after automated reformat during the
move to media/gpu.

BUG=608241
TEST=compile
NOPRESUBMIT=true
==========

to

==========
media/gpu: restore some of the reformatting done during migration to media/gpu.

This change is purely cosmetic.

Restore some of the formatting lost after automated reformat during the
move to media/gpu.

BUG=608241
TEST=compile
NOPRESUBMIT=true

Committed: https://crrev.com/d6a739e430d351bf40bf175f1ca15f5f28007d6f
Cr-Commit-Position: refs/heads/master@{#394318}
==========

[commit-bot: I haz the power, 2016-05-18 03:18:14]

Patchset 2 (id:??) landed as
https://crrev.com/d6a739e430d351bf40bf175f1ca15f5f28007d6f
Cr-Commit-Position: refs/heads/master@{#394318}