Update to Pipeline Metadata and Decoder Stream for Orientation Data

Update to Pipeline Metadata and Decoder stream
to track the set of video rotation metadata. This 
will later be used to correctly orient rotated videos.

BUG=47554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282236

[suderman, 2014-07-02 00:24:21]

Here is the separated CL for tracking rotation metadata through video decoder
config and pipeline metadata. Try server is running atm.

[scherkus (not reviewing), 2014-07-02 01:44:47]

+xhwang for some feedback

https://codereview.chromium.org/363813002/diff/1/media/base/video_decoder_con...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/363813002/diff/1/media/base/video_decoder_con...
media/base/video_decoder_config.h:139: int rotation() const;
xhwang: do you think rotation() should be part of a video decoder config? should
we perhaps considering sticking this on the DemuxerStream interface?

https://codereview.chromium.org/363813002/diff/1/media/base/video_decoder_con...
media/base/video_decoder_config.h:140: void set_rotation(int rotation);
while FFmpeg may prefer to use an int, lets make it easier on downstream
consumers of this information by defining an enum that is a strict subset of
values we support

for example, something like:
enum VideoRotation {
  ROTATION_0,
  ROTATION_90,
  ROTATION_180,
  ROTATION_270,
};

[suderman, 2014-07-07 19:43:48]

On 2014/07/02 01:44:47, scherkus wrote:
> +xhwang for some feedback
> 
>
https://codereview.chromium.org/363813002/diff/1/media/base/video_decoder_con...
> File media/base/video_decoder_config.h (right):
> 
>
https://codereview.chromium.org/363813002/diff/1/media/base/video_decoder_con...
> media/base/video_decoder_config.h:139: int rotation() const;
> xhwang: do you think rotation() should be part of a video decoder config?
should
> we perhaps considering sticking this on the DemuxerStream interface?
> 
>
https://codereview.chromium.org/363813002/diff/1/media/base/video_decoder_con...
> media/base/video_decoder_config.h:140: void set_rotation(int rotation);
> while FFmpeg may prefer to use an int, lets make it easier on downstream
> consumers of this information by defining an enum that is a strict subset of
> values we support
> 
> for example, something like:
> enum VideoRotation {
>   ROTATION_0,
>   ROTATION_90,
>   ROTATION_180,
>   ROTATION_270,
> };

I've added a VideoRotation enum and changed the rotation variables to match.

[scherkus (not reviewing), 2014-07-07 20:52:38]

https://codereview.chromium.org/363813002/diff/20001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/363813002/diff/20001/media/base/pipeline.h#ne...
media/base/pipeline.h:42: rotation(media::kRotate0) {}
we're inside the media namsepace so we can drop media:: prefixes

https://codereview.chromium.org/363813002/diff/20001/media/base/pipeline.h#ne...
media/base/pipeline.h:47: media::VideoRotation rotation;
ditto

https://codereview.chromium.org/363813002/diff/20001/media/base/video_decoder...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/363813002/diff/20001/media/base/video_decoder...
media/base/video_decoder_config.h:69: enum VideoRotation { kRotate0 = 0,
kRotate90, kRotate180, kRotate270 };
let's move this into its own file (media/base/video_rotation.h), add some
comments, and use MACRO_STYLE for the names as per
http://dev.chromium.org/developers/coding-style#Naming

we'll tack this onto the DemuxerStream interface as a video_rotation() getter
since rotation information isn't really part of a video decoder configuration
(i.e., video decoders don't make any use of the information)

https://codereview.chromium.org/363813002/diff/20001/media/ffmpeg/ffmpeg_comm...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/363813002/diff/20001/media/ffmpeg/ffmpeg_comm...
media/ffmpeg/ffmpeg_common.cc:420: DCHECK(rotation == 0 || rotation == 90 ||
rotation == 180 || rotation == 270);
are we certain FFmpeg can't spit out values other than these 4?

if we aren't, then we should fail gracefully in the default section below (e.g.,
LOG(ERROR) << "Unsupported video rotation: " << rotation;)

[suderman, 2014-07-07 22:31:57]

On 2014/07/07 20:52:38, scherkus wrote:
> https://codereview.chromium.org/363813002/diff/20001/media/base/pipeline.h
> File media/base/pipeline.h (right):
> 
>
https://codereview.chromium.org/363813002/diff/20001/media/base/pipeline.h#ne...
> media/base/pipeline.h:42: rotation(media::kRotate0) {}
> we're inside the media namsepace so we can drop media:: prefixes
> 
>
https://codereview.chromium.org/363813002/diff/20001/media/base/pipeline.h#ne...
> media/base/pipeline.h:47: media::VideoRotation rotation;
> ditto
Integrated

>
https://codereview.chromium.org/363813002/diff/20001/media/base/video_decoder...
> File media/base/video_decoder_config.h (right):
> 
>
https://codereview.chromium.org/363813002/diff/20001/media/base/video_decoder...
> media/base/video_decoder_config.h:69: enum VideoRotation { kRotate0 = 0,
> kRotate90, kRotate180, kRotate270 };
> let's move this into its own file (media/base/video_rotation.h), add some
> comments, and use MACRO_STYLE for the names as per
> http://dev.chromium.org/developers/coding-style#Naming
> 
> we'll tack this onto the DemuxerStream interface as a video_rotation() getter
> since rotation information isn't really part of a video decoder configuration
> (i.e., video decoders don't make any use of the information)
Removed from VideoDecoderConfig and added rotation data to DemuxerStream

>
https://codereview.chromium.org/363813002/diff/20001/media/ffmpeg/ffmpeg_comm...
> File media/ffmpeg/ffmpeg_common.cc (right):

>
https://codereview.chromium.org/363813002/diff/20001/media/ffmpeg/ffmpeg_comm...
> media/ffmpeg/ffmpeg_common.cc:420: DCHECK(rotation == 0 || rotation == 90 ||
> rotation == 180 || rotation == 270);
> are we certain FFmpeg can't spit out values other than these 4?
> 
> if we aren't, then we should fail gracefully in the default section below
(e.g.,
> LOG(ERROR) << "Unsupported video rotation: " << rotation;)

Currently ffmpeg only provides the these 4 options (mov.c:2615-2625) however I'm
still going to add the logging data in case this changes in the future.

[scherkus (not reviewing), 2014-07-07 22:41:11]

https://codereview.chromium.org/363813002/diff/40001/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/363813002/diff/40001/media/base/demuxer_strea...
media/base/demuxer_stream.h:85: void set_video_rotation(VideoRotation
video_rotation);
this class is a pure virtual interface and we should try to keep it that way

in other words, declare video_rotation as pure virtual and remove the
video_rotation_ member variable and update implementations (most can return
VIDEO_ROTATION_0)

https://codereview.chromium.org/363813002/diff/40001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/363813002/diff/40001/media/base/pipeline.h#ne...
media/base/pipeline.h:22: #include "media/base/video_decoder_config.h"
no longer need this include

https://codereview.chromium.org/363813002/diff/40001/media/base/video_rotation.h
File media/base/video_rotation.h (right):

https://codereview.chromium.org/363813002/diff/40001/media/base/video_rotatio...
media/base/video_rotation.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
nit: it's 2014 :)

https://codereview.chromium.org/363813002/diff/40001/media/base/video_rotatio...
media/base/video_rotation.h:5: #ifndef MEDIA_BASE_VIDEO_ROTATION_H
include guards end with a trailing _

https://codereview.chromium.org/363813002/diff/40001/media/base/video_rotatio...
media/base/video_rotation.h:10: // Enumeration to rerpesent 90 degree video
rotation for mov/mp4 videos
typo: represent

https://codereview.chromium.org/363813002/diff/40001/media/base/video_rotatio...
media/base/video_rotation.h:11: // where it can be rotated by 90 degree
intervals
nit: sentences should end with periods

https://codereview.chromium.org/363813002/diff/40001/media/base/video_rotatio...
media/base/video_rotation.h:17: };
general comment on vertical whitespace formatting: typically space out closing }
blocks with a blank line

add one here as well as after the closing namespace brace

https://codereview.chromium.org/363813002/diff/40001/media/base/video_rotatio...
media/base/video_rotation.h:18: }
we close off namespaces with two spaces + a comment

}  // namespace media

https://codereview.chromium.org/363813002/diff/40001/media/base/video_rotatio...
media/base/video_rotation.h:19: #endif
ditto for include guards

#endif  // MEDIA_BASE_VIDEO_ROTATION_H_

https://codereview.chromium.org/363813002/diff/40001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/363813002/diff/40001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:122: DCHECK(rotation == 0 || rotation == 90 ||
rotation == 180 ||
if you gracefully handle it below there's no need to DCHECK

https://codereview.chromium.org/363813002/diff/40001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:125: // Translate to the appropriate enum
comment isn't very helpful -- I'd remove it

[suderman, 2014-07-07 23:50:58]

I've made the move to virtual functions. It may make sense to have
set_video_rotation be unique for FFMpegDemuxerStream as it is they only
DemuxerStream that can have have a variation in the video rotation. At this
point set_rotation exists for all other demuxer streams but has no effects.

[scherkus (not reviewing), 2014-07-07 23:58:28]

On 2014/07/07 23:50:58, suderman wrote:
> I've made the move to virtual functions. It may make sense to have
> set_video_rotation be unique for FFMpegDemuxerStream as it is they only
> DemuxerStream that can have have a variation in the video rotation. At this
> point set_rotation exists for all other demuxer streams but has no effects.

Yeah -- as mentioned we don't want to give clients of DemuxerStream (e.g.,
decoders, pipeline) the ability to change the value. We can remove that method
entirely.

[scherkus (not reviewing), 2014-07-07 23:58:45]

few more nitty things -- we're getting there!

https://codereview.chromium.org/363813002/diff/60001/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/363813002/diff/60001/media/base/demuxer_strea...
media/base/demuxer_stream.h:85: virtual void set_video_rotation(VideoRotation
video_rotation) = 0;
clients shouldn't be able to change the rotation -- I'd just remove this method
entirely

https://codereview.chromium.org/363813002/diff/60001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/363813002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:95: last_packet_timestamp_(kNoTimestamp()),
don't forget to initialize video_rotation_!

https://codereview.chromium.org/363813002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:116: // Extract rotation metadata
add period to ... or just remove the comment entirely

https://codereview.chromium.org/363813002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:123: set_video_rotation(VIDEO_ROTATION_90);
instead of these setters, just set video_rotation_ directly

https://codereview.chromium.org/363813002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:135: set_video_rotation(VIDEO_ROTATION_0);
nit: since you'll have initialized this in the ctor, you can drop this

[scherkus (not reviewing), 2014-07-07 23:59:03]

... also your CL description will need updating to reflect the change to no
longer use VideoDecoderConfig

[suderman, 2014-07-08 16:04:00]

I've removed set_video_rotation, made sure to initialize video_rotation_ and
updated the CL description as needed.

[scherkus (not reviewing), 2014-07-08 17:17:47]

awesome work!

now we just need a test :)

can you generate some test content of mp4s that are rotated? you can use a
single frame png and I believe ffmpeg can help set the right bit.

rileya@ has been generating test content so sync up with him. he can also land
the files for you when you verify they're working.

then write a PipelineIntegrationTest that inspects that the saved metadata_
contains the correct parsed rotation information

note that for MP4 content you should be using ffmpeg_branding=Chrome and
use_proprietary_codecs=1 GYP_DEFINES

[suderman, 2014-07-09 21:51:39]

I added the pipelineIntegrationTests for 0, 90, 180, and 270 degrees. I also
updated the ffmpeg demuxer unittests to check for the same property on the
FFmpegDemuxerStream objects.

[scherkus (not reviewing), 2014-07-09 22:08:37]

can you email me the test files?

CQ doesn't support binary files, so I'll manually land them for you

[scherkus (not reviewing), 2014-07-09 22:15:16]

more nits -- thanks for writing tests!

we'll also be renaming the files from .mov -> .mp4

https://codereview.chromium.org/363813002/diff/160001/media/base/video_rotati...
File media/base/video_rotation.h (right):

https://codereview.chromium.org/363813002/diff/160001/media/base/video_rotati...
media/base/video_rotation.h:10: // Enumeration to represent 90 degree video
rotation for mov/mp4 videos
pedantic nit: mov/mp4 -> MP4

(we're pretty good at maintaining our capitalization on acronyms!)

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:920: const char * file =
"bear_rotate_0.mov";
* goes with the type

GOOD: const char* foo
BAD: const char * foo
BAD: const char *foo

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:926:
ASSERT_TRUE(stream->video_rotation() == media::VIDEO_ROTATION_0);
gtest's {EXPECT,ASSERT}_EQ() uses (expected, actual) ordering of arguments

also drop media:: prefixes inside media namespace

in other words:

ASSERT_EQ(VIDEO_ROTATION_0, stream->video_rotation());

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:930: const char * file =
"bear_rotate_90.mov";
ditto

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:936:
ASSERT_TRUE(stream->video_rotation() == media::VIDEO_ROTATION_90);
ditto

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:940: const char * file =
"bear_rotate_180.mov";
ditto

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:946:
ASSERT_TRUE(stream->video_rotation() == media::VIDEO_ROTATION_180);
ditto

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:950: const char * file =
"bear_rotate_270.mov";
ditto

https://codereview.chromium.org/363813002/diff/160001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:956:
ASSERT_TRUE(stream->video_rotation() == media::VIDEO_ROTATION_270);
ditto

https://codereview.chromium.org/363813002/diff/160001/media/filters/pipeline_...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/363813002/diff/160001/media/filters/pipeline_...
media/filters/pipeline_integration_test.cc:1397: TEST_F(PipelineIntegrationTest,
Rotated_Metadata_0) {
since these use MP4 types they should be protected with #if
defined(USE_PROPRIETARY_CODECS) / #endif

https://codereview.chromium.org/363813002/diff/160001/media/filters/pipeline_...
media/filters/pipeline_integration_test.cc:1399:
ASSERT_TRUE(metadata_.video_rotation == media::VIDEO_ROTATION_0);
ditto for ASSERT_EQ() here +below

[suderman, 2014-07-09 22:58:09]

Changes integrated.

[scherkus (not reviewing), 2014-07-09 23:01:28]

one tiny nit -- but lgtm otherwise

https://codereview.chromium.org/363813002/diff/180001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/363813002/diff/180001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:921: CreateDemuxer(file);
any reason why you don't simply inline the "bear_rotate_xxx.mp4" string inside
the call to CreateDemuxer()?

[suderman, 2014-07-09 23:09:06]

Force of habit. Any file names / strings for tests I previously declared at the
top of the test to define the scope. Reverted to use a hardcoded string.

[suderman, 2014-07-09 23:21:16]

The CQ bit was checked by suderman@chromium.org

[commit-bot: I haz the power, 2014-07-09 23:23:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/suderman@chromium.org/363813002/200001

[commit-bot: I haz the power, 2014-07-10 03:24:21]

Change committed as 282236

[wolenetz, 2014-07-16 01:16:50]

https://codereview.chromium.org/363813002/diff/200001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/363813002/diff/200001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.h:282: VideoRotation video_rotation_;
post-commit drive-by: Is this member unused?

[scherkus (not reviewing), 2014-07-16 17:53:46]

https://codereview.chromium.org/363813002/diff/200001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/363813002/diff/200001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.h:282: VideoRotation video_rotation_;
On 2014/07/16 01:16:50, wolenetz wrote:
> post-commit drive-by: Is this member unused?

looks like it isn't!

suderman: can you send out a CL removing this?

[suderman, 2014-07-16 18:27:19]

Uploaded in https://codereview.chromium.org/394703004/

[Julien Isorce, 2017-09-22 08:01:02]

Hi, what about moving VideoRotation to VideoDecoderConfig instead of having it
floating everywhere ?

https://codereview.chromium.org/363813002/diff/200001/media/base/demuxer_stre...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/363813002/diff/200001/media/base/demuxer_stre...
media/base/demuxer_stream.h:85: 
Hi, why VideoRotation has not fall into VideoDecoderConfig ?

https://codereview.chromium.org/363813002/diff/200001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/363813002/diff/200001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:136: }
This block should have been in chromium/src/media/ffmpeg/ffmpeg_common.cc
because it also parses stream->metadata there. And put VideoRotation in
VideoDecoderConfig.

[DaleCurtis, 2017-09-22 16:52:18]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2017-09-22 16:52:19]

This is a very old CL, so please either fire a bug or submit a CL with your
proposed changes and we can take a look.

[Julien Isorce, 2017-09-25 09:55:41]

On 2017/09/22 16:52:19, DaleCurtis wrote:
> This is a very old CL, so please either fire a bug or submit a CL with your
> proposed changes and we can take a look.

Done: crbug.com/768317