Remove EMPTY from VideoFrame::Format.

Remove EMPTY from VideoFrame::Format.

It was being used to indicate end-of-stream, which was sort of an abuse of the format enum. Instead, we now have a CreateEOSFrame(), rather than CreateEmptyFrame(), and an |end_of_stream_| member.

BUG=313827
TBR=danakj

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

[rileya (GONE FROM CHROMIUM), 2013-11-04 21:42:59]

https://codereview.chromium.org/57403003/diff/30001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/57403003/diff/30001/media/base/video_frame.cc...
media/base/video_frame.cc:441: bool VideoFrame::IsEndOfStream() const {
Should this be renamed end_of_stream()? Since it is now just a getter for
end_of_stream_.

[scherkus (not reviewing), 2013-11-04 23:40:28]

another thing we should consider is to DCHECK(!end_of_stream()) most of the
public API similar to when I tightened up other parts of our APIs:
http://crrev.com/177525
http://crrev.com/177550

https://codereview.chromium.org/57403003/diff/30001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/57403003/diff/30001/media/base/video_frame.cc...
media/base/video_frame.cc:349: end_of_stream_(format == UNKNOWN && timestamp ==
kNoTimestamp() &&
do you know whether it's possible to pass in values to other VideoFrame
construction methods to get this to become true?

we've had issues in the pass where clients unintentionally create EOS buffers
(e.g., some other bit of code spits out 0x0 coded_size or a kNoTimestamp())

it might be worth being explicit here and changing the VideoFrame constructor to
accept an end_of_stream_ field vs. trying to deduce it from a set of parameters
(AFAIK there are only 6 callers all of which are in this .cc, so it should be an
easy change)

https://codereview.chromium.org/57403003/diff/30001/media/base/video_frame.cc...
media/base/video_frame.cc:441: bool VideoFrame::IsEndOfStream() const {
On 2013/11/04 21:42:59, rileya wrote:
> Should this be renamed end_of_stream()? Since it is now just a getter for
> end_of_stream_.

yes + moved to .h

[scherkus (not reviewing), 2013-11-04 23:42:38]

On 2013/11/04 23:40:28, scherkus wrote:
> another thing we should consider is to DCHECK(!end_of_stream()) most of the
> public API similar to when I tightened up other parts of our APIs:
> http://crrev.com/177525
> http://crrev.com/177550

NOTE: I'd do the DCHECK(!end_of_stream()) stuff as a separate CL since you're
bound to find some random bits of code that will be breaking this assumption.

[rileya (GONE FROM CHROMIUM), 2013-11-05 00:20:35]

https://codereview.chromium.org/57403003/diff/30001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/57403003/diff/30001/media/base/video_frame.cc...
media/base/video_frame.cc:349: end_of_stream_(format == UNKNOWN && timestamp ==
kNoTimestamp() &&
On 2013/11/04 23:40:29, scherkus wrote:
> do you know whether it's possible to pass in values to other VideoFrame
> construction methods to get this to become true?
> 
> we've had issues in the pass where clients unintentionally create EOS buffers
> (e.g., some other bit of code spits out 0x0 coded_size or a kNoTimestamp())
> 
> it might be worth being explicit here and changing the VideoFrame constructor
to
> accept an end_of_stream_ field vs. trying to deduce it from a set of
parameters
> (AFAIK there are only 6 callers all of which are in this .cc, so it should be
an
> easy change)

I went with adding an explicit param for EOS in the constructor.

https://codereview.chromium.org/57403003/diff/30001/media/base/video_frame.cc...
media/base/video_frame.cc:441: bool VideoFrame::IsEndOfStream() const {
On 2013/11/04 23:40:29, scherkus wrote:
> On 2013/11/04 21:42:59, rileya wrote:
> > Should this be renamed end_of_stream()? Since it is now just a getter for
> > end_of_stream_.
> 
> yes + moved to .h

Done (quite a few callers needed to be find/replaced).

[scherkus (not reviewing), 2013-11-05 00:24:42]

https://codereview.chromium.org/57403003/diff/90001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/57403003/diff/90001/media/base/video_frame.h#...
media/base/video_frame.h:247: bool eos_frame);
nit: end_of_stream

(good practice to have ctor var names match the member var names, sans
underscore suffix)

https://codereview.chromium.org/57403003/diff/90001/media/filters/decrypting_...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/57403003/diff/90001/media/filters/decrypting_...
media/filters/decrypting_video_decoder_unittest.cc:58: MATCHER(end_of_stream,
"end of stream") {
this should still be IsEndOfStream()

(unix_hacker only applies to stuff like inlined getters/setters on classes)

[rileya (GONE FROM CHROMIUM), 2013-11-05 00:37:27]

https://codereview.chromium.org/57403003/diff/90001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/57403003/diff/90001/media/base/video_frame.h#...
media/base/video_frame.h:247: bool eos_frame);
On 2013/11/05 00:24:43, scherkus wrote:
> nit: end_of_stream
> 
> (good practice to have ctor var names match the member var names, sans
> underscore suffix)

Changed.

https://codereview.chromium.org/57403003/diff/90001/media/filters/decrypting_...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/57403003/diff/90001/media/filters/decrypting_...
media/filters/decrypting_video_decoder_unittest.cc:58: MATCHER(end_of_stream,
"end of stream") {
On 2013/11/05 00:24:43, scherkus wrote:
> this should still be IsEndOfStream()
> 
> (unix_hacker only applies to stuff like inlined getters/setters on classes)

Ahh, alrighty then, fixed.

[scherkus (not reviewing), 2013-11-05 01:21:04]

lgtm

[commit-bot: I haz the power, 2013-11-05 01:36:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/57403003/160001

[commit-bot: I haz the power, 2013-11-05 01:36:54]

Failed to apply patch for cc/resources/video_resource_updater.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/video_resource_updater.cc
  Hunk #1 FAILED at 77.
  Hunk #2 FAILED at 104.
  2 out of 2 hunks FAILED -- saving rejects to file
cc/resources/video_resource_updater.cc.rej

Patch:       cc/resources/video_resource_updater.cc
Index: cc/resources/video_resource_updater.cc
diff --git a/cc/resources/video_resource_updater.cc
b/cc/resources/video_resource_updater.cc
index
23869847e691c7348ae6dcb033009d38414489b3..335b711c3254387714dc64161addc838be05c40c
100644
--- a/cc/resources/video_resource_updater.cc
+++ b/cc/resources/video_resource_updater.cc
@@ -77,7 +77,6 @@ bool VideoResourceUpdater::VerifyFrame(
 
     // Unacceptable inputs. ¯\(°_o)/¯
     case media::VideoFrame::UNKNOWN:
-    case media::VideoFrame::EMPTY:
     case media::VideoFrame::I420:
       break;
   }
@@ -104,7 +103,6 @@ static gfx::Size SoftwarePlaneDimension(
         return gfx::ToFlooredSize(gfx::ScaleSize(coded_size, 0.5f, 1.f));
 
       case media::VideoFrame::UNKNOWN:
-      case media::VideoFrame::EMPTY:
       case media::VideoFrame::I420:
       case media::VideoFrame::NATIVE_TEXTURE:
 #if defined(GOOGLE_TV)

[commit-bot: I haz the power, 2013-11-05 19:24:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/57403003/160001

[commit-bot: I haz the power, 2013-11-05 21:46:47]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-05 22:16:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/57403003/160001

[commit-bot: I haz the power, 2013-11-05 23:31:02]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-07 22:29:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/57403003/160001

[commit-bot: I haz the power, 2013-11-08 05:32:43]

Change committed as 233776