Clean up in content/media/

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

Trunk
mozilla19
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

The code in content/media/nsBuiltin* is crufty, it's about time we cleaned up some of our technical debt before marching on. Most of this stuff is left over from when we assumed our decoder backends would inherit from nsMediaDecoder, rather than from nsBuiltinDecoderReader like we ended up doing.

Goals:

1. nsBuiltinDecoderStateMachine inherits from nsDecoderStateMachine, but we don't use nsDecoderStateMachine interface anywhere. We can just flatten these into a single interface/implementation.

2. nsBuiltinDecoder inherits from nsMediaDecoder, but there's not a huge benefit from this, as the only implementor of nsMediaDecoder is nsBuiltinDecoder. These can be flattened into a single interface/implementation.

3. Almost all of nsBuiltinDecoder's fields are public...

4. Remove "nsBuiltin" from the names of these classes.

In terms of renaming our classes, some options I've been thinking of include moving our classes into either the mozilla or mozilla::media namespace. We can then have names like: mozilla::MediaDecoder, or mozilla::media::Decoder, *::DecoderStateMachine, and *::Reader.

We already have existing classes which are in the mozilla namespace (such as mozilla::MediaResource), so it's less work to follow that lead and have mozilla::MediaDecoder, mozilla::MediaDecoderStateMachine, mozilla::MediaReader, etc.

If we move into mozilla::media, then we need to decide how many classes to move in. Ideally we'd move everything in. If we go with mozilla::media however I think it'd be a bit funny to have a mozilla::media::Cache class however, perhaps we should rename nsMediaCache to mozilla::media::ResourceCache?

Or we could just do a global s/nsBuiltin/nsMedia/ on the class names...

kinetik,roc,doublec: What would you guys prefer in terms of namespaces/renames? Now's a good time to bikeshed.
I prefer a flatter namespace so mozilla::MediaDecoder would  be by preference. I pretty much agree with everything else you've suggested.
Yes. Nested namespaces have turned out poorly.
While we're doing this, can we get rid of the nsOggDecoder etc subclasses and find another way to pass the reader in? It's kinda dumb to have these subclasses that only exist so we can override CreateStateMachine and pass in a different kind of reader.
Absolutely! I'd been thinking about this, but forgot to mention it.
Remove nsBuiltinDecoder::GetDecodeState(), so that everything that includes nsBuiltinDecoder.h doesn't have to include nsBuiltinDecoderStateMachine.h in order to know about nsDecoderStateMachine::State. Once we flatten the nsDecoderStateMachine and nsBuiltinDecoderStateMachine into one class, this will remove circular dependencies in our include graph, and simplify it.
Attachment #678588 - Flags: review?(roc)
Attachment #678588 - Attachment description: Patch: Remove nsBuiltinDecoder::GetDecodeState() → Patch 1 v1: Remove nsBuiltinDecoder::GetDecodeState()
Un-templatify nsBuiltinDecoderReader::DecodeToFirstData() so that we don't need to know nsBuiltinDecoderStateMachine's interface in nsBuiltinDecoderReader.h. This means that nsBuiltinDecoderReader.h doesn't need to include nsBuiltinDecoderStateMachine.h when we move the state machine declaration from nsBuiltinDecoder.h to nsBuiltinDecoderStateMachine.h (in a later patch). nsBuiltinDecoderStateMachine.h needs to include nsBuiltinDecoderReader.h, so if nsBuiltinDecoderReader.h needed to include nsBuiltinDecoderStateMachine.h we'd have an include cycle, which is bad.
Attachment #678589 - Flags: review?(roc)
Remove virtual from media state machine methods, since they're not inherited anyway.
Attachment #678591 - Flags: review?(roc)
These includes are hangers on since the initial landings of media element way back in http://hg.mozilla.org/mozilla-central/rev/82a78cd809c4 . They're not needed anymore.
Attachment #678592 - Flags: review?(roc)
(In reply to Chris Pearce (:cpearce) from comment #7)
> Created attachment 678590 [details] [diff] [review]
> Patch 3 v1: Flatten nsBultinDecoderStateMachine and nsDecoderStateMachine
> into a single class.
> 
> No more nsDecoderStateMachine.

Why keep Builtin in the name? Or are you planning to remove it later?
Yes I'm planning on changing the name later; I want to keep the number of logical changes per patch low to make them easier to follow and review.
Comment on attachment 678588 [details] [diff] [review]
Patch 1 v1: Remove nsBuiltinDecoder::GetDecodeState()

Review of attachment 678588 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsBuiltinDecoderStateMachine.h
@@ +265,5 @@
>    bool HaveEnoughDecodedVideo();
>  
> +  // Returns true if the state machine has shutdown or is in the process of
> +  // shutting down. The decoder monitor must be held while calling this.
> +  bool IsShutdown();

virtual bool IsShutdown() MOZ_OVERRIDE;
Attachment #678588 - Flags: review?(roc) → review+
Comment on attachment 678589 [details] [diff] [review]
Patch 2 v1: Un-templatify nsBuiltinDecoderReader::DecodeToFirstData()

Review of attachment 678589 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsBuiltinDecoderReader.cpp
@@ +371,5 @@
> +    }
> +    eof = !DecodeAudioData();
> +  }
> +  AudioData* d = nullptr;
> +  return (d = mAudioQueue.PeekFront()) ? d : nullptr;}

newline before }
Attachment #678589 - Flags: review?(roc) → review+
(In reply to Chris Pearce (:cpearce) from comment #11)
> Yes I'm planning on changing the name later; I want to keep the number of
> logical changes per patch low to make them easier to follow and review.

I assume you're actually going to rename to mozilla::MediaDecoderStateMachine.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 678588 [details] [diff] [review]
> Patch 1 v1: Remove nsBuiltinDecoder::GetDecodeState()
> 
> Review of attachment 678588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/nsBuiltinDecoderStateMachine.h
> @@ +265,5 @@
> >    bool HaveEnoughDecodedVideo();
> >  
> > +  // Returns true if the state machine has shutdown or is in the process of
> > +  // shutting down. The decoder monitor must be held while calling this.
> > +  bool IsShutdown();
> 
> virtual bool IsShutdown() MOZ_OVERRIDE;

Sure. I'll need to remove this in patch 4 though.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> I assume you're actually going to rename to
> mozilla::MediaDecoderStateMachine.

That seems the obvious candidate. If anyone has any other ideas, now is the time to table them.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7663e68657
https://hg.mozilla.org/integration/mozilla-inbound/rev/24870ebc9665
https://hg.mozilla.org/integration/mozilla-inbound/rev/c62f225d0dbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/84a1afc5b15e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0e69962962

Landed these to reduce my un-bitrotting burden and so I can get these patches out of my queue.

I will continue with the state machine and reader renames in a follow up bug.

Flattening nsMediaDecoder, nsBuiltinDecoder and all nsBuiltinDecoder's backend subclasses was a little tricker, especially since the DASH backend actually needs to inherit from nsBuiltinDecoder to change its behaviour, so I'm going to hold off removing all nsBuiltinDecoder's subclasses.

I'll try and merge nsMediaDecoder and nsBuiltinDecoder before moving on though.
Summary: Clean up nsBuiltin* in content/media/ → Clean up in content/media/
Unfortunately, these were causing mochitest-1 failures. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd15d6741bff

https://tbpl.mozilla.org/php/getParsedLog.php?id=16801721&tree=Mozilla-Inbound

126227 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | bug523816.ogv duration (0.766659) should be around 0.533
126232 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | bug523816.ogv current time at end: 0.766659 should be: 0.533
126266 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | bug498380.ogv duration (0.766659) should be around 0.533
126268 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | bug498380.ogv duration (0.766659) should be around 0.533
126271 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | bug498380.ogv duration (0.766659) should be around 0.533
126274 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | bug498380.ogv duration (0.766659) should be around 0.533
126276 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | bug498380.ogv current time at end: 0.766659 should be: 0.533
Comment on attachment 678589 [details] [diff] [review]
Patch 2 v1: Un-templatify nsBuiltinDecoderReader::DecodeToFirstData()

Review of attachment 678589 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsBuiltinDecoderReader.cpp
@@ +351,5 @@
> +      if (mDecoder->GetStateMachine()->IsShutdown()) {
> +        return nullptr;
> +      }
> +    }
> +    bool unused;

The test failure is caused because this needs to be:

bool unused = false;
You need to log in before you can comment on or make changes to this bug.