1160169 - [FFOS] MP4Reader cannot play video-only track file.

Status: RESOLVED WONTFIX

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

Description

[Blake Wu [:bwu][:blakewu]]

Device:
Flame with the latest code base, central. 

STR:
1. Go to people.mozilla.org/~cpearce/fmp4 
2. Download the video file to sdcard. 
3. Go to video APP and play it. 

Actual: 
It will show nothing. 

Expected: 
Show video frames.  

Repro rate:
100%.

After some quick check, perhaps it is due to the 0-length duration obtained from Demuxer[1].

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.cpp?from=MP4Reader.cpp&case=true#472

Comment 1

[Blake Wu [:bwu][:blakewu]]

If I use original Android demuxer(MPEG4Extractor) via MediaOmxReader, this problem cannot be seen. However, if I use gecko's MPEG4Extractor modified from Android's via MP4Reader, this problem can be seen. It looks the changes in bug 1093567 causes this problem.

Comment 2

[Blake Wu [:bwu][:blakewu]]

The value of duration in 'mdhd' box in the test content mentioned in comment 0 is 0. Normally in this case MPEG4Extractor should check the duration in 'sidx' box. But after the patch in bug 1065850 is landed, it will not read that far to get the information of 'sidx' when calling readMetaData().

This kind of video-only track file only cannot be played locally. Continue to check why it can be played in video streaming case even its duration is 0.

Comment 3

[Blake Wu [:bwu][:blakewu]]

The reason that playing via browser cannot hit this problem is there is no seek operation from the beginning. For video App, it will do seek to 0 at first. But after seeking, it will go to *Completed* state in SeekCompleted() and the playback is ended[1] due to mEndTime is 0 which is set with duration value in SetDuration() [2].


[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp?from=MediaDecoderStateMachine.cpp&case=true#2456  
[2]https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp?from=MediaDecoderStateMachine.cpp&case=true#1451

Comment 4

[Blake Wu [:bwu][:blakewu]]

After discussing with ayang, 'sidx' box is not a mandatory box defined at page#9[1]. So technically there exists some files without this box. So we still need to find a way to let 0-duration video file be played.
This bug will still try to get duration from sidx box. And another following bug 1164380 will be intended to try to play 0-duration file. 

[1]www.3gpp.org/ftp/Inbox/LSs_from_external_bodies/ISO_IEC_JTC1_SG29_WG11/29n12310.zip

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #2)
> The value of duration in 'mdhd' box in the test content mentioned in comment
> 0 is 0. Normally in this case MPEG4Extractor should check the duration in
> 'sidx' box. But after the patch in bug 1065850 is landed, it will not read
> that far to get the information of 'sidx' when calling readMetaData().
> 
> This kind of video-only track file only cannot be played locally. Continue
> to check why it can be played in video streaming case even its duration is 0.

from the spec:
"Valid top-level boxes such as pdin, free, and sidx are allowed to appear before the moov box. These boxes must be accepted and ignored by the user agent and are not considered part of the initialization segment in this specification."

A file with no duration should be considered as infinity as per MSE spec. Now we do that when used with MSE (since bug 1119757), but not the fragmented file is played outside directly.

That file plays for me however with the duration set to 0 and the duration is then updated as data is downloaded.

Why does your video app does seek(0) ? maybe we should just mark that file as non-seekable (I thought it would have been, didn't you change something there?)

Comment 6

[Blake Wu [:bwu][:blakewu]]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #2)
> > The value of duration in 'mdhd' box in the test content mentioned in comment
> > 0 is 0. Normally in this case MPEG4Extractor should check the duration in
> > 'sidx' box. But after the patch in bug 1065850 is landed, it will not read
> > that far to get the information of 'sidx' when calling readMetaData().
> > 
> > This kind of video-only track file only cannot be played locally. Continue
> > to check why it can be played in video streaming case even its duration is 0.
> 
> from the spec:
> "Valid top-level boxes such as pdin, free, and sidx are allowed to appear
> before the moov box. These boxes must be accepted and ignored by the user
> agent and are not considered part of the initialization segment in this
> specification."
> 
> A file with no duration should be considered as infinity as per MSE spec.
> Now we do that when used with MSE (since bug 1119757), but not the
> fragmented file is played outside directly.
> 
> That file plays for me however with the duration set to 0 and the duration
> is then updated as data is downloaded.
> 
> Why does your video app does seek(0) ? maybe we should just mark that file
> as non-seekable (I thought it would have been, didn't you change something
> there?)
Usually video app seeks to 0 since it wants to play from the start. I didn't not change anything in MP4Reader, demuxer, and MDSM.

Comment 7

[Jean-Yves Avenard [:jya]]

I'm referring to this change:

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

It could cause a video that would normally be marked as non-seekable to now be seekable.

Comment 8

[Blake Wu [:bwu][:blakewu]]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> I'm referring to this change:
> 
> https://hg.mozilla.org/mozilla-central/rev/b091b66b50a7
> 
> It could cause a video that would normally be marked as non-seekable to now
> be seekable.
I think that change is no problem and not related to this bug. It makes the check of seekability from reading via transport side (HTTP request) from reading via media data itself.

Comment 9

[Blake Wu [:bwu][:blakewu]]

Attachment: Read-duration-from-sidx-box-if-duration-is-0.patch

ajones, 
Need your help to review since previously you had modified the codes that this patch is going to change.
The reason to modify it is explained in comment 2.

Comment 10

[Jean-Yves Avenard [:jya]]

I don't believe this is the way to do it. sidx duration do not represent the file duration.
The problem is elsewhere in MP4Reader. It should set a duration of 0 as infinity (just like MediaSourceReader does)

Comment 11

[Blake Wu [:bwu][:blakewu]]

Attachment: v2-Read-duration-from-sidx-box-if-duration-is-0.patch

Update a new patch.

Comment 12

[Blake Wu [:bwu][:blakewu]]

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> I don't believe this is the way to do it. sidx duration do not represent the
> file duration.
I think it can and original Android's MPEG4Extractor use it as well. Let me dig into the ISO base media file format spec to make sure. 
> The problem is elsewhere in MP4Reader. It should set a duration of 0 as
> infinity (just like MediaSourceReader does)
If sidx's duration represents the file duration, we should use it. Why not?

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #12)
> (In reply to Jean-Yves Avenard [:jya] from comment #10)
> > I don't believe this is the way to do it. sidx duration do not represent the
> > file duration.
> I think it can and original Android's MPEG4Extractor use it as well. Let me
> dig into the ISO base media file format spec to make sure. 
> > The problem is elsewhere in MP4Reader. It should set a duration of 0 as
> > infinity (just like MediaSourceReader does)
> If sidx's duration represents the file duration, we should use it. Why not?

Because the spec says that it must be ignored?

A sidx represents the segment duration (ISO 14496-12 8.16.3.1).
Per ISO BMFF: a sidx must be ignored (http://w3c.github.io/media-source/isobmff-byte-stream-format.html#iso-media-segments: : "Valid top-level boxes such as pdin, free, and sidx are allowed to appear before the moov box. These boxes must be accepted and *ignored* by the user agent."

We're trying to get around a particular problem around a particular file that happens to normally only be ever played through MSE. Are you aware of more files like this that aren't designed to be played through MSE?

Should we decide to get around the ISO spec, and not ignore that box, then that's not how we should read the sidx either.

The MP4Metadata already has mechanism in place to default to another duration should the main duration be 0.
It would be much better to set it there. As we are about to replace libstagefright with our own metadata parser, the fix would still be available there.

I've voiced my opinion, now obviously you can do as you wish.

Comment 14

[Blake Wu [:bwu][:blakewu]]

(In reply to Jean-Yves Avenard [:jya] from comment #13)
:
> Because the spec says that it must be ignored?
> 
> A sidx represents the segment duration (ISO 14496-12 8.16.3.1).
> Per ISO BMFF: a sidx must be ignored
> (http://w3c.github.io/media-source/isobmff-byte-stream-format.html#iso-media-
> segments: : "Valid top-level boxes such as pdin, free, and sidx are allowed
> to appear before the moov box. These boxes must be accepted and *ignored* by
> the user agent."
It looks wired to me. I cannot figure out the reasons why spec asks to accept and ignore those boxes. 
> 
> We're trying to get around a particular problem around a particular file
> that happens to normally only be ever played through MSE. Are you aware of
> more files like this that aren't designed to be played through MSE?
I hit this problem in local playback, *not* in video streaming via MSE. Those fragmented mp4 files with video-only track playing in our video app should have this problem.
> 
> Should we decide to get around the ISO spec, and not ignore that box, then
> that's not how we should read the sidx either.
How should we read sidx? 
> 
> The MP4Metadata already has mechanism in place to default to another
> duration should the main duration be 0.
Where in the codes are your referring to?
> It would be much better to set it there. As we are about to replace
> libstagefright with our own metadata parser, the fix would still be
> available there.
> 
> I've voiced my opinion, now obviously you can do as you wish.
Thanks a lot for your opinions and quick feedback!:-)

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #14)
> It looks wired to me. I cannot figure out the reasons why spec asks to
> accept and ignore those boxes. 

just means that we shouldn't error as per the Segment Parser Algorithm.

> > 
> > We're trying to get around a particular problem around a particular file
> > that happens to normally only be ever played through MSE. Are you aware of
> > more files like this that aren't designed to be played through MSE?
> I hit this problem in local playback, *not* in video streaming via MSE.
> Those fragmented mp4 files with video-only track playing in our video app
> should have this problem.

wait, you're telling me that your video application can generate such files, or are you referring to the videos on cpearce's site, which are just extracted from YouTube's MSE conformance test.

If the video app generate such videos, well it's the generation that should be fixed.
those videos aren't valid.

> > Should we decide to get around the ISO spec, and not ignore that box, then
> > that's not how we should read the sidx either.
> How should we read sidx? 

In MP4Metadata we look for a duration key, if not present we look for another.
Could add another one.

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8605696 [details] [diff] [review]
v2-Read-duration-from-sidx-box-if-duration-is-0.patch

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

I'm OK with landing this change because it restores the earlier behaviour of libstagefright. It may make sense to pull out the sidx boxes (convert them to free space?) in the container parser. You're probably best to work out the details with jya and ask him for the next review. However I'm going to r- it because we need to stop landing behavioural changes to MPEG4Extractor that don't have accompanying tests.

Comment 17

[Blake Wu [:bwu][:blakewu]]

(In reply to Jean-Yves Avenard [:jya] from comment #15)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #14)
> > It looks wired to me. I cannot figure out the reasons why spec asks to
> > accept and ignore those boxes. 
> 
> just means that we shouldn't error as per the Segment Parser Algorithm.
> 
> > > 
> > > We're trying to get around a particular problem around a particular file
> > > that happens to normally only be ever played through MSE. Are you aware of
> > > more files like this that aren't designed to be played through MSE?
> > I hit this problem in local playback, *not* in video streaming via MSE.
> > Those fragmented mp4 files with video-only track playing in our video app
> > should have this problem.
> 
> wait, you're telling me that your video application can generate such files,
> or are you referring to the videos on cpearce's site, which are just
> extracted from YouTube's MSE conformance test.
It's not my video app, is FFOS default video player app, so it cannot generate files. 
The videos I got is from cpearce's site, but technically those files are not special and are in fragmented mp4 format. So it's possible to find this kind of video file in the internet.

Comment 18

[Blake Wu [:bwu][:blakewu]]

Since the usage of sidx is different in MSE and non-MSE case. I would like to add one more key, e.g. kKeySidxDuration, to store the sidx duration read in MPEG4Extractor.cpp. And add one more duration, mSidxDuration, in the class TrackInfo. Besides, we need to have a way to know if MP4Reader is playing MSE content or not. And we can chose the sidx duration when mDuration is 0 for non-MSE case.   

jya,
How do you think?

Comment 19

[Jean-Yves Avenard [:jya]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #17)
> It's not my video app, is FFOS default video player app, so it cannot
> generate files. 
> The videos I got is from cpearce's site, but technically those files are not
> special and are in fragmented mp4 format. So it's possible to find this kind
> of video file in the internet.

those files have no reasons to be outside of MSE; fragmented MP4 is designed to allow for multiple fragments.
But yeah, it's possible to find them, from site like cpearce's which simply give you what you normally find MSE.
That file on its own is invalid and doesn't follow the ISO BMFF spec.

Comment 20

[Jean-Yves Avenard [:jya]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #18)
> Since the usage of sidx is different in MSE and non-MSE case. I would like
> to add one more key, e.g. kKeySidxDuration, to store the sidx duration read
> in MPEG4Extractor.cpp. And add one more duration, mSidxDuration, in the
> class TrackInfo. Besides, we need to have a way to know if MP4Reader is
> playing MSE content or not. And we can chose the sidx duration when
> mDuration is 0 for non-MSE case.   
> 
> jya,
> How do you think?

There's no need to do that.
TrackInfo is an abstract class, there should be nothing in there specific to a particular format.

There's already logic in place to read the duration from the SIDX in libstagefright.
The SIDX duration will set kKeyDuration in the Track's metadata if other durations are found to be 0.
http://mxr.mozilla.org/mozilla-central/source/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#2086

The issue however, is that stagefright stop parsing at the moov if the data isn't encrypted.
http://mxr.mozilla.org/mozilla-central/source/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#916

We just need to continue parsing until we find a sidx if we have no duration yet.
The problem with this however (and same with your patch above), is that if you have no sidx box you end up parsing the entire data. Would need to stop parsing before.

MP4Reader doesn't need to know if it's within MSE either (and MP4Reader is going away anyway).

Again, this is not a file you're going to encounter outside MSE, it isn't BMFF compliant.

Comment 21

[Blake Wu [:bwu][:blakewu]]

(In reply to Jean-Yves Avenard [:jya] from comment #20)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #18)
> > Since the usage of sidx is different in MSE and non-MSE case. I would like
> > to add one more key, e.g. kKeySidxDuration, to store the sidx duration read
> > in MPEG4Extractor.cpp. And add one more duration, mSidxDuration, in the
> > class TrackInfo. Besides, we need to have a way to know if MP4Reader is
> > playing MSE content or not. And we can chose the sidx duration when
> > mDuration is 0 for non-MSE case.   
> > 
> > jya,
> > How do you think?
> 
> There's no need to do that.
> TrackInfo is an abstract class, there should be nothing in there specific to
> a particular format.
> 
> There's already logic in place to read the duration from the SIDX in
> libstagefright.
> The SIDX duration will set kKeyDuration in the Track's metadata if other
> durations are found to be 0.
> http://mxr.mozilla.org/mozilla-central/source/media/libstagefright/
> frameworks/av/media/libstagefright/MPEG4Extractor.cpp#2086
> 
> The issue however, is that stagefright stop parsing at the moov if the data
> isn't encrypted.
> http://mxr.mozilla.org/mozilla-central/source/media/libstagefright/
> frameworks/av/media/libstagefright/MPEG4Extractor.cpp#916
> 
> We just need to continue parsing until we find a sidx if we have no duration
> yet.
> The problem with this however (and same with your patch above), is that if
> you have no sidx box you end up parsing the entire data. Would need to stop
> parsing before.
Yeah. That's what originally I tended to do in my patch. But you mention MSE spec needs to accept and *ignore* sidx box. If we want to do so, I think we can add another key to store sidx duration and set it via SetMediaDuration from MP4Reader::ReadMetadata() if it is non-MSE case. For MSE case, we can use the original mDuration to set. That's the comment 18 for.  
> 
> MP4Reader doesn't need to know if it's within MSE either (and MP4Reader is
> going away anyway).
If we want to do what I just mention above, we need to find a way to let MP4Reader know if it is in MSE case or not. 
> 
> Again, this is not a file you're going to encounter outside MSE, it isn't
> BMFF compliant.
I cannot tell why that file is not BMFF compliant. You mean having a sidx box?

Comment 22

[Jean-Yves Avenard [:jya]]

You are attempting to solve this problem the wrong way round.
Let's go back to the actual cause on why that file doesn't play. It's not that the file doesn't have a duration, or that the duration is defined as 0.
The video doesn't play because the player attempt to seek into it before playback starts. This is wrong. What if the file didn't start at 0, or that the file was unseekable? Or that the file was a MP4 with the samples index at the end of the file (like an iPhone generates).
It would cause playback to either not start, or take an excessive amount of time with lots of unecessary networking seek requests.
The video player shouldn't seek before starting playback.

We are attempting to clean the code, abstracting what can be abstracted, removing all the hack for a particular platform, and for particular media, making as much as possible content agnostic as part of bug 1148292. It's not to re-add new work arounds as soon as the older ones are removed.

Now back to reading the SIDX. It's not the MSE spec that states you have to accept, but ignore SIDX boxes when reading the metadata. It's the BMFF spec (ISO 14496-12). This spec covers all MP4s.
SIDX doesn't contain the duration of the media. It's an index, it's an optional box. What it indexes is the same as what the following moof contains. It contains the duration of the segment that follow.

So say you now read the SIDX to get the "duration". Great that particular file now plays. Did you fix the problem for all files? no. What if the file didn't have an SIDX?
You're back to the beginning of the problem: the video player attempts to seek, in either a non-seekable file or a file not properly giving its duration. The fragmented MP4 doesn't have to provide its duration because that file was designed to be used with MSE only. MSE can set the duration in JS.

Reading the SIDX is akin to looking at the last sample in the moof and looking at its timestamp. That would give you the duration too.
This is far more reliable, works with all content, can be done without messing with the dangerous libstagefright.

It is in my opinion, the wrong way to start putting work around for files that weren't intended to be played on their own, and applying a band-aid without first solving the root of the problem.

As I have stated multiple times, reading the SIDX is not the proper way to solve this problem. It can be fixed more elegantly, in a more universal fashion.

I'm happy to take on this bug.

Comment 23

[Blake Wu [:bwu][:blakewu]]

(In reply to Jean-Yves Avenard [:jya] from comment #22)
> You are attempting to solve this problem the wrong way round.
> Let's go back to the actual cause on why that file doesn't play. It's not
> that the file doesn't have a duration, or that the duration is defined as 0.
> The video doesn't play because the player attempt to seek into it before
> playback starts. This is wrong. What if the file didn't start at 0, or that
No matter browser or video APP should be able to do seek before playing. Client can preload MetaData and set currentTime or fastSeek without click the play button. I am talking about the non-MSE case. Why is it wrong?  
> the file was unseekable? Or that the file was a MP4 with the samples index
> at the end of the file (like an iPhone generates).
For this kind of files our mDemuxer->CanSeek() should return false. If not, it would be a bug.
> It would cause playback to either not start, or take an excessive amount of
> time with lots of unecessary networking seek requests.
> The video player shouldn't seek before starting playback.
Again. It is allowed to let client to do so. Or are you saying MSE case? What I am saying it is for non-MSE case. As you know currently MP4Reader is using for MSE and non-MSE as well. 
> 
> We are attempting to clean the code, abstracting what can be abstracted,
> removing all the hack for a particular platform, and for particular media,
> making as much as possible content agnostic as part of bug 1148292. It's not
> to re-add new work arounds as soon as the older ones are removed.
Agree! What I am trying to do is not a workaround I think :). 
> 
> Now back to reading the SIDX. It's not the MSE spec that states you have to
> accept, but ignore SIDX boxes when reading the metadata. It's the BMFF spec
> (ISO 14496-12). This spec covers all MP4s.
> SIDX doesn't contain the duration of the media. It's an index, it's an
> optional box. What it indexes is the same as what the following moof
> contains. It contains the duration of the segment that follow.
> 
> So say you now read the SIDX to get the "duration". Great that particular
> file now plays. Did you fix the problem for all files? no. What if the file
> didn't have an SIDX?
I know SIDX is optional, so I fired bug 1164380 to fix it.  
> You're back to the beginning of the problem: the video player attempts to
> seek, in either a non-seekable file or a file not properly giving its
> duration. The fragmented MP4 doesn't have to provide its duration because
> that file was designed to be used with MSE only. MSE can set the duration in
> JS.
Fragmented MP4 could be originally designed for DASH via MSE, but this kind of file could be downloaded from internet as a single file to be played and this format may be adopted for more recorders due to its flexibility to streaming. This bug is trying to let fragmented mp4 file played in browser and video player app, *not* via MSE. 
> 
> Reading the SIDX is akin to looking at the last sample in the moof and
> looking at its timestamp. That would give you the duration too.
> This is far more reliable, works with all content, can be done without
> messing with the dangerous libstagefright.
> 
> It is in my opinion, the wrong way to start putting work around for files
> that weren't intended to be played on their own, and applying a band-aid
> without first solving the root of the problem.
The root of this problem is can we use SIDX's duration? I am going to find out. 
> 
> As I have stated multiple times, reading the SIDX is not the proper way to
> solve this problem. It can be fixed more elegantly, in a more universal
> fashion. 
As I know Android uses this SIDX duration as well and I used ffmpeg to demux this file and found ffmpeg can get the duration as well. Maybe it gets it from sidx. I will check it too. 
May I know some details about the more elegantly and universal fashion? 
> 
> I'm happy to take on this bug.
Thanks for your help! But I would like to continue checking how to fix this bug and keep posting what I found. And I think we are not on the same page yet. It looks like you stands in the viewpoint of MSE and I stand in the viewpoint of normal playback and believe sidx's duration is useful for us. Maybe we can use Vidyo to save some time on communications on bugzilla. Maybe you can get a Flame and play the same file from cpearce's side and you will see what I am observing now. Anyway I really appreciate your time to discuss with me here!

Comment 24

[Jean-Yves Avenard [:jya]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #23)

> No matter browser or video APP should be able to do seek before playing.

not if a media is non-seekable.
that won't work.

> Client can preload MetaData and set currentTime or fastSeek without click
> the play button. I am talking about the non-MSE case. Why is it wrong?

because that assumes you have a fixed duration. That won't work for non-seekable media, such as a stream or with non-seekable server.

The problem here is that your playback won't even start. A file without duration is perfectly valid ; how would you play a stream of MP3 where you can't think with your current player ?

> Agree! What I am trying to do is not a workaround I think :). 

It is.. you are attempting to solve in such a manner that it will only fix it for THAT file, and only files that have a SIDX.

What if the file doesn't have a SIDX?

A fix that works for a single case, and only in a set of very narrow circumstances is a workaround in my book, and I consider this as bad.

> I know SIDX is optional, so I fired bug 1164380 to fix it.  

which is just as invalid as this one.

the MDSM is perfectly able to play a file with a duration of 0 ; unlike what the title states.
What it's unable to do is SEEK in a file with an invalid duration. That's only because you can only seek within the buffered range as per spec.

> Fragmented MP4 could be originally designed for DASH via MSE, but this kind
> of file could be downloaded from internet as a single file to be played and
> this format may be adopted for more recorders due to its flexibility to
> streaming. This bug is trying to let fragmented mp4 file played in browser
> and video player app, *not* via MSE. 

how is a single MP4 made of multiple fragments concatenated together make for better flexibility than a plain MP4 ??? It will have the same size (actually slightly bigger) the same features and play in the same way.

What you are attempting to do isn't fixing the problem: it's getting around it by reading a field (SIDX) that the spec clearly states to ignore.
That's the wrong approach.


> As I know Android uses this SIDX duration as well and I used ffmpeg to demux
> this file and found ffmpeg can get the duration as well. Maybe it gets it
> from sidx. I will check it too. 

libstagefright doesn't read the sidx when it comes to reading the init segment. It's the same as what we do (and it's the same code). It stops at the moov.

ffmpeg doesn't read the sidx. It reads the init segment; If it has a duration it will use it, if not it falls back to demuxing the first frame, check the start time, then demux the last frame and use the difference between end and start and use that as duration.
(this is ffmpeg mp4 parser: http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/mov.c;h=ed2afd421552826b0b1a9ebb23104b960665666c;hb=HEAD#l3478 you'll find no code about sidx in there)

This is what I'm suggesting we do too.

You now would have a case where this file's duration would be calculated properly and it will work for any files without a duration clearly set in their init segment INCLUDING THOSE WITHOUT SIDX

It will work with MSE like usual and plain files.

> May I know some details about the more elegantly and universal fashion? 

This is the step I described above (and in my earlier comments)

cpearce may want to add something about this.

> Maybe we can use Vidyo to save some time on communications on bugzilla.
> Maybe you can get a Flame and play the same file from cpearce's side and you
> will see what I am observing now. Anyway I really appreciate your time to
> discuss with me here!

Whistler is only a few weeks away, we can talk about this then.

I want the same thing as you do: a better firefox and firefox OS. Glad we have the same goals. Just a minor disagreement on the road to take us there :)

Comment 25

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

We could just get rid of the logic that says a 0 duration media file is unplayable.

Comment 26

[Jean-Yves Avenard [:jya]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #25)
> We could just get rid of the logic that says a 0 duration media file is
> unplayable.

there's no such logic...
A file with a duration of 0 is playable. MSE used to report any durations non set as 0 too (until bug 1119757).

And we can seek too, except that seeking will give an error as you're outside the buffered range.
Seems that the FFOS player doesn't handle that case properly

Comment 27

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The demuxer should be able to seek so it must be aborting the seek before that.

Comment 28

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #6)
> Usually video app seeks to 0 since it wants to play from the start. I didn't
> not change anything in MP4Reader, demuxer, and MDSM.

By default, Gecko starts playback of a <video> element starts at the start of a video, and we report the start as 0; we adjust video timestamps to report that.


(In reply to Jean-Yves Avenard [:jya] from comment #24)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #23)
> 
> > No matter browser or video APP should be able to do seek before playing.
> 
> not if a media is non-seekable.
> that won't work.

On this point I agree with Blake; we should make our best effort to play any file. Web developers don't read specs. That is, at internet scale, you can expect people to do the Wrong Thing, and we should try to make it Just Work.



> how is a single MP4 made of multiple fragments concatenated together make
> for better flexibility than a plain MP4 ??? 

The use case I recall reading (in the ISO fmp4 spec IIRC) is a camera that wants to do one-pass encoding data to a file, and if it used non-fragmented it would have to either leave room at the start of the file for the index, and seek back to write it, or write the index at the end (like the iPhone). 

I don't know how common this practice is.


> ffmpeg doesn't read the sidx. It reads the init segment; If it has a
> duration it will use it, if not it falls back to demuxing the first frame,
> check the start time, then demux the last frame and use the difference
> between end and start and use that as duration.
> (this is ffmpeg mp4 parser:
> http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/mov.c;
> h=ed2afd421552826b0b1a9ebb23104b960665666c;hb=HEAD#l3478 you'll find no code
> about sidx in there)
> 
> This is what I'm suggesting we do too.

I think we should do this; have the demuxer report the first and last samples' timestamp, and use that to calculate the duration.

jya: maybe you should take this bug and do that, and ask Blake to test the patch.

Comment 29

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #28)
> (In reply to Jean-Yves Avenard [:jya] from comment #24)
> > ffmpeg doesn't read the sidx. It reads the init segment; If it has a
> > duration it will use it, if not it falls back to demuxing the first frame,
> > check the start time, then demux the last frame and use the difference
> > between end and start and use that as duration.
> > (this is ffmpeg mp4 parser:
> > http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/mov.c;
> > h=ed2afd421552826b0b1a9ebb23104b960665666c;hb=HEAD#l3478 you'll find no code
> > about sidx in there)
> > 
> > This is what I'm suggesting we do too.
> 
> I think we should do this; have the demuxer report the first and last
> samples' timestamp, and use that to calculate the duration.
I agree too! Originally I was wondering if we can get the duration from SIDX. But this way is better and definitely can get the right duration. 
> 
> jya: maybe you should take this bug and do that, and ask Blake to test the
> patch.
I'm ok to un-assign myself since we are on the same page to find out a duration. For the seek before playback or the mechanism to handle non-seekable content, lets discuss in the workweek.