Bug Report about mp4 module

[Xietong LU]

The ngx_http_mp4_update_stsc_atom function has some bug.

[maxim]

Probably. But could you please provide more information about the bug you discovered?

[Xietong LU]

Replying to maxim:

Probably. But could you please provide more information about the bug you discovered?

I think there are two bugs and I just found out how to fix one of them.
In line 2491, whenever trak->chunk_samples is true, the variable entries should increase 1.
So entries++; should be added.
I didn't test it yet because I can't compile nginx with my computer.

[Maxim Dounin]

Could you please instead provide a description of the problem you are seeing, and steps to reproduce it? I think you're right and something like this

--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -2500,6 +2500,8 @@ found:
 
     if (trak->chunk_samples) {
 
+        entries++;
+
         first = &trak->stsc_chunk_entry;
         ngx_mp4_set_32value(first->chunk, 1);
         ngx_mp4_set_32value(first->samples, samples - trak->chunk_samples);

is indeed needed to correctly handle non-keyframe seeks, but it would be good to see the actual problem before trying to fix it.

[Xietong LU]

Replying to Maxim Dounin:

Could you please instead provide a description of the problem you are seeing, and steps to reproduce it? I think you're right and something like this

--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -2500,6 +2500,8 @@ found:
 
     if (trak->chunk_samples) {
 
+        entries++;
+
         first = &trak->stsc_chunk_entry;
         ngx_mp4_set_32value(first->chunk, 1);
         ngx_mp4_set_32value(first->samples, samples - trak->chunk_samples);

is indeed needed to correctly handle non-keyframe seeks, but it would be good to see the actual problem before trying to fix it.

I use an mp4 file for testing. Inside the sample to chunk box, the original entries are

Entry{firstChunk=1, samplePerChunk=13, sampleDescriptionIndex=1}
Entry{firstChunk=2, samplePerChunk=12, sampleDescriptionIndex=1}
Entry{firstChunk=167, samplePerChunk=9, sampleDescriptionIndex=1}

Then I use wget with different start time to download the regenerated mp4 files. When the start time is 0.1, where the start sample is 2nd one, the new entries are

Entry{firstChunk=1, samplePerChunk=11, sampleDescriptionIndex=1}
Entry{firstChunk=2, samplePerChunk=13, sampleDescriptionIndex=1}
Entry{firstChunk=2, samplePerChunk=12, sampleDescriptionIndex=1}

(I'm using the mp4 parser from the website: ​http://code.google.com/p/mp4parser/)
Other information is as follow.
type: stsc
size: 64
version: 0
entries: 3

You can find 2 bugs here.

1. Entry{firstChunk=167, samplePerChunk=9, sampleDescriptionIndex=1} is missing. Actually, this entry is inside the file from the size of this box, but the number of entries is 3. Then the parser didn't show it out.
2. Entry{firstChunk=2, samplePerChunk=13, sampleDescriptionIndex=1} should be deleted. It happened when the start sample is located in the last chunk specified by an entry. For example, when the start sample are from 1983 - 1993, similar problem will happen again.

My English is poor and I can't express it very clearly.

[Xietong LU]

I think the second problem can be solved by this way

+       if (trak->chunk_samples&& (start_sample/samples == next_chunk - chunk)){
+	   ngx_mp4_set_32value(entry->samples, samples - trak->chunk_samples);
+	}
        else if (trak->chunk_samples) {

		entries++; 
		...

[Maxim Dounin]

Could you please provide the mp4 file in question for testing?

[Xietong LU]

Replying to Maxim Dounin:

Could you please provide the mp4 file in question for testing?

The original file.
​https://docs.google.com/open?id=0B3Wt0yFGIKL1M21fTDEtM2lUR1k
The output file.
​https://docs.google.com/open?id=0B3Wt0yFGIKL1WE16Sm5hb19oX0k

[Maxim Dounin]

Ok, thanks. The following patch seems to resolve both issues:

--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -2498,7 +2498,13 @@ found:
 
     ngx_mp4_set_32value(entry->chunk, 1);
 
-    if (trak->chunk_samples) {
+    if (trak->chunk_samples && next_chunk - trak->start_chunk == 2) {
+
+        /* last chunk in the entry */
+
+        ngx_mp4_set_32value(entry->samples, samples - trak->chunk_samples);
+
+    } else if (trak->chunk_samples) {
 
         first = &trak->stsc_chunk_entry;
         ngx_mp4_set_32value(first->chunk, 1);
@@ -2514,6 +2520,7 @@ found:
 
         ngx_mp4_set_32value(entry->chunk, 2);
 
+        entries++;
         atom_size += sizeof(ngx_mp4_stsc_entry_t);
     }
 

Is it looks ok for you?

[Xietong LU]

Replying to Maxim Dounin:

Ok, thanks. The following patch seems to resolve both issues:

--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -2498,7 +2498,13 @@ found:
 
     ngx_mp4_set_32value(entry->chunk, 1);
 
-    if (trak->chunk_samples) {
+    if (trak->chunk_samples && next_chunk - trak->start_chunk == 2) {
+
+        /* last chunk in the entry */
+
+        ngx_mp4_set_32value(entry->samples, samples - trak->chunk_samples);
+
+    } else if (trak->chunk_samples) {
 
         first = &trak->stsc_chunk_entry;
         ngx_mp4_set_32value(first->chunk, 1);
@@ -2514,6 +2520,7 @@ found:
 
         ngx_mp4_set_32value(entry->chunk, 2);
 
+        entries++;
         atom_size += sizeof(ngx_mp4_stsc_entry_t);
     }
 

Is it looks ok for you?

yes. It looks ok.

[Maxim Dounin]

In [4688/nginx]:

Mp4: fixed non-keyframe seeks in some cases (ticket #175).

Number of entries in stsc atom was wrong if we've added an entry to
split a chunk.

Additionally, there is no need to add an entry if we are going to split
last chunk in an entry, it's enough to update the entry we already have.
Previously new entry was added and old one was left as is, resulting in
incorrect entry with zero chunks which might confuse some software.

[Maxim Dounin]

Fix committed, thanks.

[Maxim Dounin]

In [4728/nginx]:

Merge of r4688, r4689, r4706:

*) Mp4: fixed non-keyframe seeks in some cases (ticket #175).

Number of entries in stsc atom was wrong if we've added an entry to
split a chunk.

Additionally, there is no need to add an entry if we are going to split
last chunk in an entry, it's enough to update the entry we already have.
Previously new entry was added and old one was left as is, resulting in
incorrect entry with zero chunks which might confuse some software.

*) Mp4: fixed streaming if moov atom is at buffer edge.