Closed Bug 1100499 Opened 10 years ago Closed 10 years ago

[EME] Add record enumerating API to GMPStorage

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Adobe requested adding an API to GMPStorage to enumerate the records on disk.

I think we can do this like so:

1. Change the way we hash record name strings into filenames for the records' files on disk. Currently we just use Mozilla::Hash(), which is lossy, meaning we can't reproduce the record name. We can instead Base64 encode the record name and use that as the records's filename on disk. Then we can retrieve the record name by un-Base64 encoding the record's filename.
2. Add the enumerating API; enumerate the files in the parent process in the GMP's per-origin storage dir. Since the API we expose to GMPs must be async, we will be returning a snapshot of the records that were present at the time the enumeration ran in the parent process. By the time it gets into the child process, the snapshot may be out of date. Apparently that's OK with Adobe.
Does a record name container invalid characters for a file name?
We currently hash the record name and use that as the file name. The hash outputs an uint32_t, and we just use the printable representation of that number as the filename.

With my step 1 above, we'll Base64 encode the record name, which will work as filenames, provided we replace the '/' characters that our Base64Encode function can insert.
Attached patch Patch v1Splinter Review
* Switch to using Base64 encoded record names as the filenames for records.
* Added a GMPRecordIterator interface to gmp-storage.h, which iterates over a snapshot of the record names in storage. We don't handle records changing while the iterator is alive.
* Added a GMPCreateRecordIterator function to gmp-platform, to create a GMPRecordIterator.
* Implement iteration in GMPStorageParent.
* Tests!
Attachment #8525018 - Flags: review?(rjesup)
Comment on attachment 8525018 [details] [diff] [review]
Patch v1

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

::: dom/media/gmp-plugin/gmp-test-decryptor.cpp
@@ +309,5 @@
> +  std::string response("record-names ");
> +  bool first = true;
> +  const char* name = nullptr;
> +  uint32_t len = 0;
> +  while (GMP_SUCCEEDED(aRecordIterator->GetName(&name, &len))) {

shouldn't GetName() use size_t instead of uint32_t for len?

::: dom/media/gmp/GMPPlatform.cpp
@@ +198,5 @@
> +CreateRecordIterator(RecvGMPRecordIteratorPtr aRecvIteratorFunc,
> +                     void* aUserArg)
> +{
> +  if (sMainLoop != MessageLoop::current()) {
> +    NS_WARNING("GMP called CreateRecord() on non-main thread!");

Shouldn't this be strong than a WARNING?   NS_ASSERTION or MOZ_ASSERT?

::: dom/media/gmp/GMPStorageChild.cpp
@@ +272,5 @@
> +GMPStorageChild::EnumerateRecords(RecvGMPRecordIteratorPtr aRecvIteratorFunc,
> +                                  void* aUserArg)
> +{
> +  if (mPlugin->GMPMessageLoop() != MessageLoop::current()) {
> +    NS_WARNING("GMP used GMPStorage on non-main thread.");

ditto

@@ +276,5 @@
> +    NS_WARNING("GMP used GMPStorage on non-main thread.");
> +    return GMPGenericErr;
> +  }
> +  if (mShutdown) {
> +    NS_WARNING("GMPStorage used after it's been shutdown!");

ditto

@@ +296,5 @@
> +  {
> +    mRecordNames.Sort();
> +  }
> +
> +  virtual GMPErr GetName(const char** aOutName, uint32_t* aOutNameLength) MOZ_OVERRIDE {

size_t

::: dom/media/gmp/gmp-api/gmp-storage.h
@@ +119,5 @@
> +public:
> +  // Retrieve the name for the current record.
> +  // Returns GMPNoErr if successful, or GMPEndOfEnumeration if iteration has
> +  // reached the end.
> +  virtual GMPErr GetName(const char ** aOutName, uint32_t * aOutNameLength) = 0;

size_t
Attachment #8525018 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #5)
> Comment on attachment 8525018 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8525018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/gmp-plugin/gmp-test-decryptor.cpp
> @@ +309,5 @@
> > +  std::string response("record-names ");
> > +  bool first = true;
> > +  const char* name = nullptr;
> > +  uint32_t len = 0;
> > +  while (GMP_SUCCEEDED(aRecordIterator->GetName(&name, &len))) {
> 
> shouldn't GetName() use size_t instead of uint32_t for len?

The only place we use size_t in the gmp-api instead of fixed-size types is for GMPAudioCodec::mExtraDataLen.

I would prefer to be consistent and explicit about the size of types we're using.


> 
> ::: dom/media/gmp/GMPPlatform.cpp
> @@ +198,5 @@
> > +CreateRecordIterator(RecvGMPRecordIteratorPtr aRecvIteratorFunc,
> > +                     void* aUserArg)
> > +{
> > +  if (sMainLoop != MessageLoop::current()) {
> > +    NS_WARNING("GMP called CreateRecord() on non-main thread!");
> 
> Shouldn't this be strong than a WARNING?   NS_ASSERTION or MOZ_ASSERT?

Sure, this can be a MOZ_ASSERT. I would still leave the return there, for the benefit of release builds.
OK, summary of discussion with Jesup on IRC is, we agree that we should ideally be using size_t, since when we need to recompile for x64 the sizeof pointers changes anyway, and the size_t should be reliably defined.

OpenH264 currently ships on x64 Linux, so we'd have to be very careful about how we'd roll out this change. Follow up.

I'll stick with uint32_t for now.
(In reply to Chris Pearce (:cpearce) from comment #6)
> > Shouldn't this be strong than a WARNING?   NS_ASSERTION or MOZ_ASSERT?
> 
> Sure, this can be a MOZ_ASSERT. I would still leave the return there, for
> the benefit of release builds.

Since this particular API is async, if it is called off the main thread we could dispatch a task to run it on the main thread, as we do in GMPDecryptorChild with the GMPDecyptorCallback implementation. Follow up bug...
https://hg.mozilla.org/mozilla-central/rev/405b0c00071f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.