Closed Bug 915911 Opened 6 years ago Closed 6 years ago

[Video] Video title should read from metadata instead of using filename.

Categories

(Firefox OS Graveyard :: Gaia::Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
1.3 Sprint 3 - 10/25
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: tchung, Assigned: johnhu)

References

Details

(Whiteboard: [MEDIA_TRIAGED])

Attachments

(3 files, 1 obsolete file)

when saving a video off of browser content, the video is saved with the name "videoplayback" and displayed in the title.  it would be better if the local video download retained the same name that it was saved from the web.

See screenshot.   The examples in the image were originally called "Conan" and "Iphone 5S"

Repro:
1) install 1.2 Buri nightly; build 20130912040201
2) launch browser, and go to youtube video.  note the name of the video you are downloading (ie. "Conan" or "Iphone 5S"
3) long tap the video region and "Save video"   
4) close browser,and launch video app
5) find the video you downloaded, and play it
6) Verify the video title contains "Videoplayback", instead of the actual name of the video that was hosted from the website

Expected:
- retain the name of the video from the website you downloaded from.   preferably a user friendly name, and not just "videoplayback"

AcutaL
- all videos saved are listed as "videoplayback"
We currently use filename to display the list. I will exam if the mp4 file from YouTube contains metadata information..
The best way to show the title is using the metadata from file. If we can't get it, we may use filename as fallback.
I had dumped the metadata with ffmpeg, but it doesn't contain title information. Event if we implement the metadata parser for title, we can't show friendly title for YouTube files.

Metadata:
    major_brand     : mp42
    minor_version   : 0
    compatible_brands: isommp42
    creation_time   : 2013-09-10 06:26:54
  Duration: 00:00:19.99, start: 0.000000, bitrate: 409 kb/s
    Stream #0:0(und): Video: h264 (Constrained Baseline) (avc1 / 0x31637661), yuv420p, 640x360, 309 kb/s, 29.97 fps, 29.97 tbr, 60k tbn, 59.94 tbc
    Metadata:
      handler_name    : VideoHandler
    Stream #0:1(und): Audio: aac (mp4a / 0x6134706D), 44100 Hz, stereo, fltp, 96 kb/s
    Metadata:
      creation_time   : 2013-09-10 06:26:54
      handler_name    : IsoMedia File Produced by Google, 5-11-2011
blocking-b2g: koi? → koi+
Whiteboard: [MEDIA_TRIAGED]
QA Contact: mozillamarcia.knous
Assignee: nobody → johu
Hi David, 

Our current implementation is to use "filename" as the title only. But the best way is to read the title from metadata. The followings are my investigation results and I may need to confirm the gaia implementation with you:

1. We do have metadata parsing support by gecko, but it is only for OGG, bug 763010.
2. The supported containers for video element are webm, ogg, and mp4[1]. I can use file extension to use metadata parser from gecko. But for webm and mp4, there are still opening bugs, bug 778049 and bug 778052. To support them, we may need to write our owned metadata parser.

To have the work around, should I write the metadata parsers for webm and mp4 to extract the title from metadata of container and metadata of track, like music app?? That may consume lots time to finish it.

Or, should I just link the code to mozGetMetadata and wait for those bugs landed to have full metadata parsing support??

[1] https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats
Flags: needinfo?(dflanagan)
Attached patch WIP based on mozGetMetadata (obsolete) — Splinter Review
David, 

Please also look at this one. This is the WIP patch based on mozGetMetadata. In this WIP patch, I don't care about the insufficient metadata parser, just use it. Once gecko bugs fixed, it works automatically.
John,

I like your patch that uses mozGetMetadata when it is available and falls back to the filename.

We should *not* implement our own metadata parser for this bug. We just need to give the files slightly more meaningful names than what we're getting now.

I think the problem is that whatever is downloading video file is saving it with the name "videoplayback.mp4", possibly prefixed with a random number.

Where is the code that does that?  Is it in the browser app? Or deeper in gecko? Let's change that code so it puts the files in sdcard/downloads/movies/ and then does something more intelligent for a filename.

Maybe the browser could base the filename on the URL of the video or the domain of the site the video is embedded on.

A name like "saved_video_<yyyy-mm-dd>" would be a nice compromise.  The shared/js/device_storage/get_unused_filename.js might help create a unique filename.

Really, this seems to me like a Browser bug, not a Video bug.  Dale, do you know where the save video feature is implemented?
Flags: needinfo?(dflanagan) → needinfo?(dale)
I found it. It is at [1]. It uses the last portion of url to be the file name and uses MimeMapper to guess the extension. That's why it always be Videoplayback.mp4.

I can change the filename to follow "/sdcard/downloads/saved_media_<yyyy-mm-dd>" pattern. But I also want to hear Dale's comment about this. Before that, I can provide a WIP to include it.

[1] https://github.com/mozilla-b2g/gaia/blob/afb7f95a2696a415e28dc5461022072d4270596c/apps/browser/js/browser.js#L934
BTW, it uses storage.addNamed() to save the video file. If the file is already existed, it prepend the timestamp to filename to prevent file overwriting, like the attachment.

In the description of this bug, we should use the page title to save it. But we may also need to face the same title among all pages in some websites.
As per comment 6, how about to create another bug for the construction of file name about browser app and keep this one on retrieving title from metadata about video app???
We extract the filename from the url to the media object, I would imagine that using the title of the embedded page will cause the exact same problem but likely more severe (a page with several videos embedded)

I cant think of many workarounds that would improve the current situation, I think the only reasonable solution is to parse metadata, but if someone comes up with a nicer scheme for filenames that doesnt break very common use cases thne happy to take it

Sorry for taking ages to reply, for some reason this didnt come up in my dashboard earlier
Flags: needinfo?(dale)
As per Dale's comment, I will open another bug for tracking the filename scheme issue of browser app and use this one to read the metadata from the video file.
See Also: → 922960
I had opened another bug 922960 to keep track the filename scheme issue of browser app.
Summary: [Video] Saving a video from web will not retain the video name, instead says "Videoplayback" → [Video] Video title should read from metadata instead of using filename.
Use mozGetMetadata API to read the title from metadata. This is created from WIP patch.
Attachment #809031 - Attachment is obsolete: true
Attachment #812988 - Flags: review?(dflanagan)
Comment on attachment 812988 [details]
read title from metadata

I left one suggestion on github, but this looks good to land as it is.

Note that I have not tested this myself.

My understanding, however, is that this patch is not enough, by itself, to fix the underlying bug. Does youtube actually embed metadata in its videos?  If so, then we need to get gecko to parse metadata from other video formats.  I'm not sure whether we have a bug open for that, but if so, maybe it should inherit the koi+ flag we have here.

Alternatively, we should make bug 922960 a koi+ bug.
Attachment #812988 - Flags: review?(dflanagan) → review+
Thanks David,

I know your concern. I had tried to dump the metadata from YouTube videos. They don't contain title in metadata, full dumping like:

Metadata:
    major_brand     : mp42
    minor_version   : 0
    compatible_brands: isommp42
    creation_time   : 2013-09-10 06:26:54
  Duration: 00:00:19.99, start: 0.000000, bitrate: 409 kb/s
    Stream #0:0(und): Video: h264 (Constrained Baseline) (avc1 / 0x31637661), yuv420p, 640x360, 309 kb/s, 29.97 fps, 29.97 tbr, 60k tbn, 59.94 tbc
    Metadata:
      handler_name    : VideoHandler
    Stream #0:1(und): Audio: aac (mp4a / 0x6134706D), 44100 Hz, stereo, fltp, 96 kb/s
    Metadata:
      creation_time   : 2013-09-10 06:26:54
      handler_name    : IsoMedia File Produced by Google, 5-11-2011

There is a bug already dealing with the metadata parsing of different video types in gecko, the bug 778052. I will nominate it as koi?.
merged to master:
https://github.com/mozilla-b2g/gaia/commit/60bb46e0082284d46d74d6c043003b3ef89d4351
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Uplifted 60bb46e0082284d46d74d6c043003b3ef89d4351 to:
v1.2: 212ac56f54d96b1f5b8b2b67c7a8e58b72589534
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Duplicate of this bug: 796394
You need to log in before you can comment on or make changes to this bug.