Closed Bug 1288329 Opened 8 years ago Closed 8 years ago

When transitioning over chained ogg; new metadataloaded event should be fired.

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Depends on 1 open bug)

Details

Attachments

(8 files)

see dom/media/test/test_chaining.html

Need to amend MediaFormatReader to handle chaining
Chris - do we need to continue chaining support?
Flags: needinfo?(cpearce)
Decided that we will only support chaining audio files, however the new metadataloaded won't be fired at each transition (which is against the spec anyway)
Flags: needinfo?(cpearce)
Is this something where we should try to get the spec changed? Seems like this is needed for song title changes in web radio.

I guess we probably don't support ICY metadata updates for MP3, either?
Flags: needinfo?(jyavenard)
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #1)
> Chris - do we need to continue chaining support?

In Firefox 47, 0.02% of all media elements used report that they were used for chained ogg playback:
https://mzl.la/2ahsCBE

It's pretty easy in the new media format reader to keep supporting the minimal subset of chaining that we support. The only part that requires effort to support is re-firing loaded metadata when changing links. I'd be happy to lose that.

We shouldn't remove chaining without warning. We should add webconsole logging when Ogg chaining is encountered to encourage webdevs to switch from chained oggs to WebM+MSE.

(In reply to Timothy B. Terriberry (:derf) from comment #3)
> Is this something where we should try to get the spec changed? Seems like
> this is needed for song title changes in web radio.

Streaming radio should be using MSE. Ogg chaining never took off, and we shouldn't be encouraging it to.

> I guess we probably don't support ICY metadata updates for MP3, either?

Yes, and I'm not aware of anyone asking for it. Are you aware of anyone asking for it?
> > I guess we probably don't support ICY metadata updates for MP3, either?
> 
> Yes, and I'm not aware of anyone asking for it. Are you aware of anyone asking for it?

I'm not. Everything you say makes sense. Thanks!
Flags: needinfo?(jyavenard)
So I added the metadata support to the ogg demuxer yesterday, I kind of like the support of MozMetadata, too bad it didn't take off.

On second thought, replicating the behaviour of the OggReader when it comes to metadata could be easily hacked off by having the demuxer itself firing the notification event to the decoder (JW made that very easy with his EventNotifier class).

Extending to do the same in MP3 would be trivial too, just saying.
If it's cheap, go for it.
This is not the cleanest approach, but ensures identical behavior with the OggReader when it comes to firing loadedmetadata event and handling the change of seekability.

A more universal solution could be considered involving the MediaFormatReader and changing the MediaDataDemuxer API, of interest would be adding support for a new event fired whenever we have a change of content or metadata (useful with MSE or recorded webm of a WebRTC session

Review commit: https://reviewboard.mozilla.org/r/67614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67614/
Attachment #8775430 - Flags: review?(cpearce)
Now passes all media mochitests.

Review commit: https://reviewboard.mozilla.org/r/67724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67724/
Attachment #8775430 - Attachment description: Bug 1288329: [ogg] Add support for metadata chaining in OggDemuxer. → Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.
Attachment #8775540 - Flags: review?(cpearce)
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67614/diff/1-2/
The OggReader always passed a complete ogg_packet to the vorbis decoder, ensuring that the right number of frames was be returned. In the conversion to the new architecture, this information got lost making the vorbis decoder always return more frames than normal on the last packet.

Review commit: https://reviewboard.mozilla.org/r/67786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67786/
Attachment #8775540 - Attachment description: Bug 1288329: [ogg] P2. Enable new OggDemuxer by default. → Bug 1288329: [ogg] P4. Enable new OggDemuxer by default.
Attachment #8775636 - Flags: review?(cpearce)
Attachment #8775637 - Flags: review?(cpearce)
The old OggReader only use the end time as duration of the video, ignoring the start time of the first sample. This leads to incorrect duration calculation.

Review commit: https://reviewboard.mozilla.org/r/67788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67788/
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67614/diff/2-3/
Comment on attachment 8775540 [details]
Bug 1288329: [ogg] P8. Enable new OggDemuxer by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67724/diff/1-2/
Review commit: https://reviewboard.mozilla.org/r/67886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67886/
Attachment #8775637 - Attachment description: Bug 1288329: [ogg] P3. Fix mochitest. → Bug 1288329: [ogg] P5. Adjust mochitests.
Attachment #8775540 - Attachment description: Bug 1288329: [ogg] P4. Enable new OggDemuxer by default. → Bug 1288329: [ogg] P6. Enable new OggDemuxer by default.
Attachment #8775838 - Flags: review?(cpearce)
Attachment #8775839 - Flags: review?(gsquelart)
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67614/diff/3-4/
Comment on attachment 8775636 [details]
Bug 1288329: [ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67786/diff/1-2/
Comment on attachment 8775637 [details]
Bug 1288329: [ogg] P6. Adjust mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67788/diff/1-2/
Comment on attachment 8775540 [details]
Bug 1288329: [ogg] P8. Enable new OggDemuxer by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67724/diff/2-3/
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67614/diff/4-5/
Comment on attachment 8775636 [details]
Bug 1288329: [ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67786/diff/2-3/
Comment on attachment 8775838 [details]
Bug 1288329: [ogg] P3. Never take into considerations frames prior the first keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67886/diff/1-2/
Comment on attachment 8775839 [details]
Bug 1288329: [ogg] P5. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67888/diff/1-2/
Comment on attachment 8775637 [details]
Bug 1288329: [ogg] P6. Adjust mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67788/diff/2-3/
Comment on attachment 8775540 [details]
Bug 1288329: [ogg] P8. Enable new OggDemuxer by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67724/diff/3-4/
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

https://reviewboard.mozilla.org/r/67614/#review64982

r+ with nits:

::: dom/media/ogg/OggDecoder.cpp:21
(Diff revision 5)
> +      useFormatDecoder ? new OggDemuxer(GetResource()) : nullptr;
>    RefPtr<MediaDecoderReader> reader = useFormatDecoder ?
> -      static_cast<MediaDecoderReader*>(new MediaFormatReader(this, new OggDemuxer(GetResource()), GetVideoFrameContainer())) :
> +      static_cast<MediaDecoderReader*>(new MediaFormatReader(this, demuxer, GetVideoFrameContainer())) :
>        new OggReader(this);

Style: use 2 spaces indent instead of 4.

::: dom/media/ogg/OggDemuxer.cpp:167
(Diff revision 5)
> +
> +

Double empty lines, is that intended?
Attachment #8775430 - Flags: review+
Comment on attachment 8775636 [details]
Bug 1288329: [ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer.

https://reviewboard.mozilla.org/r/67786/#review64986
Comment on attachment 8775838 [details]
Bug 1288329: [ogg] P3. Never take into considerations frames prior the first keyframe.

https://reviewboard.mozilla.org/r/67886/#review64988

r+ with nit:

In commit description: 'prior the' -> 'prior to the'.
Attachment #8775838 - Flags: review+
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

https://reviewboard.mozilla.org/r/67614/#review65006

::: dom/media/MediaDecoderReader.h:294
(Diff revision 5)
>    }
>  
>    // Notified by the OggReader during playback when chained ogg is detected.
>    MediaEventSource<void>& OnMediaNotSeekable() { return mOnMediaNotSeekable; }
>  
> +  TimedMetadataEventProducer& TimedMetadataProducer()

Return a raw ponter so you don't need NotNull<> below.

::: dom/media/ogg/OggDecoder.cpp:26
(Diff revision 5)
> +      useFormatDecoder ? new OggDemuxer(GetResource()) : nullptr;
>    RefPtr<MediaDecoderReader> reader = useFormatDecoder ?
> -      static_cast<MediaDecoderReader*>(new MediaFormatReader(this, new OggDemuxer(GetResource()), GetVideoFrameContainer())) :
> +      static_cast<MediaDecoderReader*>(new MediaFormatReader(this, demuxer, GetVideoFrameContainer())) :
>        new OggReader(this);
> +  if (useFormatDecoder) {
> +    demuxer->SetChainingEvents(WrapNotNull(&reader->TimedMetadataProducer()),

SetChainingEvents() is regarded as part of the initialization of the demuxer. Please move the call above |new MediaFormatReader| so MFR has no chance to use a half-baked object.

::: dom/media/ogg/OggDecoder.cpp:27
(Diff revision 5)
>    RefPtr<MediaDecoderReader> reader = useFormatDecoder ?
> -      static_cast<MediaDecoderReader*>(new MediaFormatReader(this, new OggDemuxer(GetResource()), GetVideoFrameContainer())) :
> +      static_cast<MediaDecoderReader*>(new MediaFormatReader(this, demuxer, GetVideoFrameContainer())) :
>        new OggReader(this);
> +  if (useFormatDecoder) {
> +    demuxer->SetChainingEvents(WrapNotNull(&reader->TimedMetadataProducer()),
> +                               WrapNotNull(&reader->MediaNotSeekableProducer()));

Why not just passing raw pointers? I can't see how NotNull<> helps here.
Attachment #8775430 - Flags: review+
Comment on attachment 8775636 [details]
Bug 1288329: [ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer.

https://reviewboard.mozilla.org/r/67786/#review65026

::: dom/media/MediaData.h:661
(Diff revision 3)
>    const CryptoSample& mCrypto;
>    RefPtr<MediaByteBuffer> mExtraData;
>  
> +  // Used by the Vorbis decoder and Ogg demuxer.
> +  // Indicates that this is the last packet of the stream.
> +  bool mEOS;

I would prefer in-class initialization so you don't need to change every constructor when adding a new member.
Attachment #8775636 - Flags: review+
Comment on attachment 8775839 [details]
Bug 1288329: [ogg] P5. Fix coding style.

https://reviewboard.mozilla.org/r/67888/#review65028

::: dom/media/ogg/OggDemuxer.h:94
(Diff revision 2)
>      SeekRange(int64_t aOffsetStart,
>                int64_t aOffsetEnd,
>                int64_t aTimeStart,
>                int64_t aTimeEnd)
> -      : mOffsetStart(aOffsetStart),
> -        mOffsetEnd(aOffsetEnd),
> +      : mOffsetStart(aOffsetStart)
> +      ,  mOffsetEnd(aOffsetEnd)

2 spaces between "," and "mOffsetEnd".
Attachment #8775839 - Flags: review+
https://reviewboard.mozilla.org/r/67614/#review65006

> SetChainingEvents() is regarded as part of the initialization of the demuxer. Please move the call above |new MediaFormatReader| so MFR has no chance to use a half-baked object.

How would that be possible as prior new MediaFormatReader the objects we want to pass to the OggDemuxer do not yet exist ?

> Why not just passing raw pointers? I can't see how NotNull<> helps here.

I felt like using something new and so I don't have to put assert in the OggDemuxer :)
https://reviewboard.mozilla.org/r/67786/#review65026

> I would prefer in-class initialization so you don't need to change every constructor when adding a new member.

not sure I follow. could you provide an example?

MediaRawData isn't supposed to change... it's unfortunate that I found no other way to pass that required data.
https://reviewboard.mozilla.org/r/67614/#review65006

> How would that be possible as prior new MediaFormatReader the objects we want to pass to the OggDemuxer do not yet exist ?

I mean:
1. RefPtr<OggDemuxer> demuxer = new OggDemuxex(...);
2. demuxer->SetChainingEvents(...);
3. reader = new MediaFormatReader(this, demuxer, ...);
https://reviewboard.mozilla.org/r/67786/#review65026

> not sure I follow. could you provide an example?
> 
> MediaRawData isn't supposed to change... it's unfortunate that I found no other way to pass that required data.

bool mEOS = false;
  So you don't need to init the member in every constructor.
https://reviewboard.mozilla.org/r/67614/#review65048

::: dom/media/ogg/OggDemuxer.cpp:714
(Diff revision 5)
>      if (mIsChained) {
>        return;
>      }
>      mIsChained = true;
>    }
> -  // @FIXME how can MediaDataDemuxer / MediaTrackDemuxer notify this has changed?
> +  if (mOnSeekableEvent) {

You won't need null-check if the member is declared as NotNull<T>.
https://reviewboard.mozilla.org/r/67614/#review65006

> I mean:
> 1. RefPtr<OggDemuxer> demuxer = new OggDemuxex(...);
> 2. demuxer->SetChainingEvents(...);
> 3. reader = new MediaFormatReader(this, demuxer, ...);

How can you call the demuxer with parameters found in the reader if the reader hasn't been created yet...
https://reviewboard.mozilla.org/r/67614/#review65006

> How can you call the demuxer with parameters found in the reader if the reader hasn't been created yet...

Oops, I see what you mean. Forget about this non-sense.
Attachment #8775430 - Flags: review?(cpearce)
Attachment #8775636 - Flags: review?(cpearce)
Attachment #8775838 - Flags: review?(cpearce)
Attachment #8775637 - Flags: review?(cpearce)
Attachment #8775540 - Flags: review?(cpearce)
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67614/diff/5-6/
Attachment #8775430 - Flags: review?(cpearce)
Attachment #8775430 - Flags: review+
Attachment #8775636 - Flags: review?(cpearce)
Attachment #8775636 - Flags: review+
Attachment #8775838 - Flags: review+ → review?(cpearce)
Attachment #8775839 - Flags: review+
Attachment #8775637 - Flags: review+ → review?(cpearce)
Attachment #8775540 - Flags: review+ → review?(cpearce)
Comment on attachment 8775636 [details]
Bug 1288329: [ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67786/diff/3-4/
Comment on attachment 8775838 [details]
Bug 1288329: [ogg] P3. Never take into considerations frames prior the first keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67886/diff/2-3/
Comment on attachment 8775839 [details]
Bug 1288329: [ogg] P5. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67888/diff/2-3/
Comment on attachment 8775637 [details]
Bug 1288329: [ogg] P6. Adjust mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67788/diff/3-4/
Comment on attachment 8775540 [details]
Bug 1288329: [ogg] P8. Enable new OggDemuxer by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67724/diff/4-5/
Attachment #8775430 - Flags: review?(cpearce)
Attachment #8775636 - Flags: review?(cpearce)
Attachment #8775838 - Flags: review?(cpearce) → review?(jwwang)
Attachment #8775637 - Flags: review?(cpearce) → review?(jwwang)
Attachment #8775540 - Flags: review?(cpearce) → review?(jwwang)
Attachment #8775838 - Flags: review?(jwwang)
Attachment #8775637 - Flags: review?(jwwang)
Attachment #8775540 - Flags: review?(jwwang)
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67614/diff/6-7/
Comment on attachment 8775636 [details]
Bug 1288329: [ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67786/diff/4-5/
Comment on attachment 8775838 [details]
Bug 1288329: [ogg] P3. Never take into considerations frames prior the first keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67886/diff/3-4/
Comment on attachment 8775839 [details]
Bug 1288329: [ogg] P5. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67888/diff/3-4/
Comment on attachment 8775637 [details]
Bug 1288329: [ogg] P6. Adjust mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67788/diff/4-5/
Comment on attachment 8775540 [details]
Bug 1288329: [ogg] P8. Enable new OggDemuxer by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67724/diff/5-6/
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a76b6e559777
[ogg] P1. Add support for metadata chaining in OggDemuxer. r=gerald,jwwang
https://hg.mozilla.org/integration/autoland/rev/a1c4d9b9de32
[ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer. r=gerald,jwwang
https://hg.mozilla.org/integration/autoland/rev/936b73eb37c6
[ogg] P3. Never take into considerations frames prior the first keyframe. r=gerald
https://hg.mozilla.org/integration/autoland/rev/1406c14098f1
[ogg] P4. Fix coding style. r=gerald,jwwang
https://hg.mozilla.org/integration/autoland/rev/49ea83d00a77
[ogg] P5. Adjust mochitests. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f33cb032cc9f
[ogg] P6. Enable new OggDemuxer by default. r=gerald
We can seek in cached data, we will rely on the seek operation to fail instead to determine if we can't or not

Review commit: https://reviewboard.mozilla.org/r/68160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68160/
Attachment #8775839 - Attachment description: Bug 1288329: [ogg] P4. Fix coding style. → Bug 1288329: [ogg] P5. Fix coding style.
Attachment #8775637 - Attachment description: Bug 1288329: [ogg] P5. Adjust mochitests. → Bug 1288329: [ogg] P6. Adjust mochitests.
Attachment #8775540 - Attachment description: Bug 1288329: [ogg] P6. Enable new OggDemuxer by default. → Bug 1288329: [ogg] P8. Enable new OggDemuxer by default.
Attachment #8776363 - Flags: review?(gsquelart)
Attachment #8776364 - Flags: review?(gsquelart)
The ogg file contains a theora video track with a flac audio track, not vorbis.
The new OggDemuxer properly ignore the tracks it knows nothing about.
This will cause the tests to use MP4 with h264/aac instead which isn't available on Windows XP, so we mark those tests are expected to fail.

Review commit: https://reviewboard.mozilla.org/r/68162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68162/
Comment on attachment 8775430 [details]
Bug 1288329: [ogg] P1. Add support for metadata chaining in OggDemuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67614/diff/7-8/
Comment on attachment 8775636 [details]
Bug 1288329: [ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67786/diff/5-6/
Comment on attachment 8775838 [details]
Bug 1288329: [ogg] P3. Never take into considerations frames prior the first keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67886/diff/4-5/
Comment on attachment 8775839 [details]
Bug 1288329: [ogg] P5. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67888/diff/4-5/
Comment on attachment 8775637 [details]
Bug 1288329: [ogg] P6. Adjust mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67788/diff/5-6/
Comment on attachment 8775540 [details]
Bug 1288329: [ogg] P8. Enable new OggDemuxer by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67724/diff/6-7/
Comment on attachment 8776363 [details]
Bug 1288329: [ogg] P4. Don't assume we can't seek in resource if transport isn't seekable.

https://reviewboard.mozilla.org/r/68160/#review65260
Attachment #8776363 - Flags: review?(gsquelart) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/725b6c6ff7f6
[ogg] P1. Add support for metadata chaining in OggDemuxer. r=gerald,jwwang
https://hg.mozilla.org/integration/autoland/rev/b1f32a906af2
[ogg/vorbis] P2. Pass extra information to the decoder so that it can perform proper trimmer. r=gerald,jwwang
https://hg.mozilla.org/integration/autoland/rev/f9dec37d375c
[ogg] P3. Never take into considerations frames prior the first keyframe. r=gerald
https://hg.mozilla.org/integration/autoland/rev/e49d5a02100e
[ogg] P4. Don't assume we can't seek in resource if transport isn't seekable. r=gerald
https://hg.mozilla.org/integration/autoland/rev/aced758afc8a
[ogg] P5. Fix coding style. r=gerald,jwwang
https://hg.mozilla.org/integration/autoland/rev/6e1765d4c1c1
[ogg] P6. Adjust mochitests. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f3e86b019fbf
[webref] P7. Fix mimetype. r=gerald
https://hg.mozilla.org/integration/autoland/rev/de5f049f1029
[ogg] P8. Enable new OggDemuxer by default. r=gerald
Flags: needinfo?(jyavenard)
Depends on: 1319543
Blocks: 1320705
No longer blocks: 1320705
Blocks: 1320705
No longer blocks: 1320705
Depends on: 1320705
No longer depends on: 1337296
Regressions: 1337296
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: