1237836 - Support MetadataTags in MediaFormatReader

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

To move the WaveReader over to the MediaFormatReader (bug 1231793) without a feature regression, the MediaFormatReader needs to support retrieving MetadataTags from demuxers.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: MozReview Request: Bug 1237836 - Add support for MetadataTags to MediaFormatReader. r=?jya

Review commit: https://reviewboard.mozilla.org/r/29969/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29969/

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8705409 [details]
MozReview Request: Bug 1237836 - Add support for MetadataTags to MediaFormatReader. r=?jya

https://reviewboard.mozilla.org/r/29969/#review26787

::: dom/media/MediaInfo.h:94
(Diff revision 1)
> +  nsTArray<MetadataTag> mTags;

wouldn't it have been simpler to use a UniquePtr<MetadataTags> there instead?

avoid having to convert back and forth?

I guess the issue would be common keys between the audio track and video track, but that's hardly going to be a problem if the only demuxer supporting it is WAV

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8705409 [details]
> MozReview Request: Bug 1237836 - Add support for MetadataTags to
> MediaFormatReader. r=?jya
> 
> https://reviewboard.mozilla.org/r/29969/#review26787
> 
> ::: dom/media/MediaInfo.h:94
> (Diff revision 1)
> > +  nsTArray<MetadataTag> mTags;
> 
> wouldn't it have been simpler to use a UniquePtr<MetadataTags> there instead?
> 
> avoid having to convert back and forth?

It would have, if nsBaseHashtable had an assignment operator, but it doesn't...

Comment 4

[Jean-Yves Avenard [:jya]]

but the object is on the heap, can't we just shift the ownership of the pointer around? where would we need to assign it ? UniquePtr won't take care of that?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Not sure what you're asking. In theory we could have different MetadataTags in each track, so we'd end up with multiple MetadatTags, so we can't just move one MetadataTags around, we need to merge each tracks' tags.

The issue with the assignment operator is that at DecodedStream.cpp:245 we assign a MediaInfo, and that would invoke the default assignment operator on MediaInfo and thus on the MetadataTags which is an nsBaseHashTable which lacks an assignment operator, so we get a compile error. I don't think we should add an assignment operator to nsBaseHashTable, as you get shallow vs deep copy issues.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bae9aee9d8a

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c223429a08eb

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Pulsebot from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c223429a08eb

Backed out for mass test failures...

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14434fbc7fe7

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f5ac99696b

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8551de969592

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d88ea69f1ad9

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d88ea69f1ad9